Feature/790 Add Magistrates Public Adult Court List style guide and PDF - #842
Conversation
|
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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (47)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Daily and Future Magistrates Adult Court List support for public and non-public variants, including shared schemas, renderers, PDF and email outputs, English/Welsh pages, service registration, artefact-search repository separation, CI configuration, and supporting tests and documentation. ChangesMagistrates adult court lists
Sequence Diagram(s)sequenceDiagram
participant Browser
participant MagistratesListController
participant ListValidator
participant MagistratesListRenderer
participant ListTemplate
Browser->>MagistratesListController: request Daily or Future list
MagistratesListController->>ListValidator: validate artefact JSON
ListValidator-->>MagistratesListController: return validation result
MagistratesListController->>MagistratesListRenderer: build header and case rows
MagistratesListRenderer-->>MagistratesListController: return rendered data
MagistratesListController->>ListTemplate: render translated list page
ListTemplate-->>Browser: return court list
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)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright E2E Test Results84 tests 52 ✅ 6m 20s ⏱️ Results for commit 5d79e86. ♻️ 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: 7
🧹 Nitpick comments (2)
libs/list-types/common/src/validation/list-type-validator.test.ts (1)
54-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent
schemaVersionin mock.The mock returns
schemaVersion: "1.0"while all other mocks in this file return"1.0.0". While the tests only assertisValid, keeping the version consistent avoids confusion if assertions are later expanded.♻️ Suggested consistency fix
- schemaVersion: "1.0" + schemaVersion: "1.0.0"libs/list-types/magistrates-public-adult-court-list/src/config.ts (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider SCREAMING_SNAKE_CASE for exported constants.
Per coding guidelines, constants declared outside function scope should use SCREAMING_SNAKE_CASE.
assetsandschemaPathcould follow this convention. Note thatmoduleRootis exempt since theapps/web/src/app.tsguideline explicitly references it by name.As per coding guidelines: "Use SCREAMING_SNAKE_CASE for constants declared outside function scope."
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88704dff-8a75-452b-a0a7-3b1e5545fb12
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
apps/web/src/app.tsapps/web/src/pages/(list-types)/magistrates-public-adult-court-list-daily/index.tsapps/web/src/pages/(list-types)/magistrates-public-adult-court-list-daily/magistrates-public-adult-court-list-daily.njkapps/web/src/pages/(list-types)/magistrates-public-adult-court-list-future/index.tsapps/web/src/pages/(list-types)/magistrates-public-adult-court-list-future/magistrates-public-adult-court-list-future.njkdocs/tickets/790/plan.mddocs/tickets/790/tasks.mddocs/tickets/790/ticket.mdlibs/list-types/common/src/index.tslibs/list-types/common/src/rendering/date-formatting.test.tslibs/list-types/common/src/rendering/date-formatting.tslibs/list-types/common/src/validation/list-type-validator.test.tslibs/list-types/common/src/validation/list-type-validator.tslibs/list-types/magistrates-public-adult-court-list/package.jsonlibs/list-types/magistrates-public-adult-court-list/src/config.tslibs/list-types/magistrates-public-adult-court-list/src/email-summary/summary-builder.test.tslibs/list-types/magistrates-public-adult-court-list/src/email-summary/summary-builder.tslibs/list-types/magistrates-public-adult-court-list/src/index.tslibs/list-types/magistrates-public-adult-court-list/src/locales/cy.tslibs/list-types/magistrates-public-adult-court-list/src/locales/en.tslibs/list-types/magistrates-public-adult-court-list/src/pdf/pdf-generator.test.tslibs/list-types/magistrates-public-adult-court-list/src/pdf/pdf-generator.tslibs/list-types/magistrates-public-adult-court-list/src/pdf/pdf-template.njklibs/list-types/magistrates-public-adult-court-list/src/rendering/renderer.test.tslibs/list-types/magistrates-public-adult-court-list/src/rendering/renderer.tslibs/list-types/magistrates-public-adult-court-list/src/schemas/magistrates-public-adult-court-list.jsonlibs/list-types/magistrates-public-adult-court-list/src/validation/json-validator.test.tslibs/list-types/magistrates-public-adult-court-list/src/validation/json-validator.tslibs/list-types/magistrates-public-adult-court-list/tsconfig.jsonlibs/notifications/src/notification/notification-service.tslibs/publication/src/processing/service.test.tslibs/publication/src/processing/service.tstsconfig.json
| <p class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;"> | ||
| {{ t.dataSource }}: {{ dataSource }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid inline styles for accessibility.
The inline style="font-size: 14px;" on line 71 may prevent users from overriding font sizes via their browser or assistive technology settings. Consider moving this to a CSS class instead.
♿ Proposed fix for inline style
- <p class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;">
+ <p class="govuk-body govuk-!-margin-top-6 govuk-!-font-size-14">📝 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 class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;"> | |
| {{ t.dataSource }}: {{ dataSource }} | |
| <p class="govuk-body govuk-!-margin-top-6 govuk-!-font-size-14"> | |
| {{ t.dataSource }}: {{ dataSource }} |
| <p class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;"> | ||
| {{ t.dataSource }}: {{ dataSource }} | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add conditional guard for empty dataSource.
The PDF template wraps this in {% if dataSource %}, but the web template always renders it. When dataSource is empty (e.g., no provenance set), this displays "Data source: " with no value.
💚 Proposed fix
- <p class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;">
- {{ t.dataSource }}: {{ dataSource }}
- </p>
+ {% if dataSource %}
+ <p class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;">
+ {{ t.dataSource }}: {{ dataSource }}
+ </p>
+ {% endif %}📝 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 class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;"> | |
| {{ t.dataSource }}: {{ dataSource }} | |
| </p> | |
| {% if dataSource %} | |
| <p class="govuk-body govuk-!-margin-top-6" style="font-size: 14px;"> | |
| {{ t.dataSource }}: {{ dataSource }} | |
| </p> | |
| {% endif %} |
| "dependencies": { | ||
| "@hmcts/list-types-common": "workspace:*", | ||
| "@hmcts/pdf-generation": "workspace:*", | ||
| "@hmcts/postgres-prisma": "workspace:*", | ||
| "nunjucks": "3.2.4" | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add @hmcts/publication to dependencies.
json-validator.ts imports validateJson and ValidationResult from @hmcts/publication, and index.ts re-exports ValidationResult from it, but @hmcts/publication is not listed in dependencies. This will cause resolution failures for consumers of this package.
🔧 Proposed fix
"dependencies": {
"`@hmcts/list-types-common`": "workspace:*",
+ "`@hmcts/publication`": "workspace:*",
"`@hmcts/pdf-generation`": "workspace:*",
"`@hmcts/postgres-prisma`": "workspace:*",
"nunjucks": "3.2.4"
},📝 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.
| "dependencies": { | |
| "@hmcts/list-types-common": "workspace:*", | |
| "@hmcts/pdf-generation": "workspace:*", | |
| "@hmcts/postgres-prisma": "workspace:*", | |
| "nunjucks": "3.2.4" | |
| }, | |
| "dependencies": { | |
| "`@hmcts/list-types-common`": "workspace:*", | |
| "`@hmcts/publication`": "workspace:*", | |
| "`@hmcts/pdf-generation`": "workspace:*", | |
| "`@hmcts/postgres-prisma`": "workspace:*", | |
| "nunjucks": "3.2.4" | |
| }, |
| const html = env.render("pdf-template.njk", { | ||
| header: renderedData.header, | ||
| listData: renderedData.listData, | ||
| dataSource: provenanceLabel, | ||
| listTitle: options.listTitle ?? (translations as { titleDaily?: string }).titleDaily ?? "", | ||
| t: translations, | ||
| pdfStyles: PDF_BASE_STYLES | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass locale to the template context for dynamic lang attribute.
The template context doesn't include locale, so pdf-template.njk hardcodes <html lang="en">. For Welsh PDFs, the lang attribute must be cy for screen readers to pronounce content correctly. This is an accessibility issue for Welsh-speaking users.
🌐 Proposed fix: pass locale and use it in the template
In pdf-generator.ts, add locale to the render context:
const html = env.render("pdf-template.njk", {
header: renderedData.header,
listData: renderedData.listData,
dataSource: provenanceLabel,
listTitle: options.listTitle ?? (translations as { titleDaily?: string }).titleDaily ?? "",
t: translations,
- pdfStyles: PDF_BASE_STYLES
+ pdfStyles: PDF_BASE_STYLES,
+ locale: options.locale
});In pdf-template.njk, line 2:
-<html lang="en">
+<html lang="{{ locale }}">📝 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 html = env.render("pdf-template.njk", { | |
| header: renderedData.header, | |
| listData: renderedData.listData, | |
| dataSource: provenanceLabel, | |
| listTitle: options.listTitle ?? (translations as { titleDaily?: string }).titleDaily ?? "", | |
| t: translations, | |
| pdfStyles: PDF_BASE_STYLES | |
| }); | |
| const html = env.render("pdf-template.njk", { | |
| header: renderedData.header, | |
| listData: renderedData.listData, | |
| dataSource: provenanceLabel, | |
| listTitle: options.listTitle ?? (translations as { titleDaily?: string }).titleDaily ?? "", | |
| t: translations, | |
| pdfStyles: PDF_BASE_STYLES, | |
| locale: options.locale | |
| }); |
| if (!pdfResult.success || !pdfResult.pdfBuffer) { | ||
| return { | ||
| success: false, | ||
| error: pdfResult.error || "PDF generation failed" | ||
| }; | ||
| } | ||
|
|
||
| return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes!); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard against missing sizeBytes before non-null assertion.
The guard on line 54 checks success and pdfBuffer but not sizeBytes. If generatePdfFromHtml returns { success: true, pdfBuffer: Buffer, sizeBytes: undefined }, the non-null assertion on line 61 passes undefined to savePdfToStorage, which could cause incorrect exceedsMaxSize results or downstream errors.
🛡️ Proposed fix: guard on sizeBytes or compute fallback
if (!pdfResult.success || !pdfResult.pdfBuffer) {
return {
success: false,
error: pdfResult.error || "PDF generation failed"
};
}
- return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes!);
+ const sizeBytes = pdfResult.sizeBytes ?? pdfResult.pdfBuffer.length;
+ return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, sizeBytes);📝 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 (!pdfResult.success || !pdfResult.pdfBuffer) { | |
| return { | |
| success: false, | |
| error: pdfResult.error || "PDF generation failed" | |
| }; | |
| } | |
| return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes!); | |
| if (!pdfResult.success || !pdfResult.pdfBuffer) { | |
| return { | |
| success: false, | |
| error: pdfResult.error || "PDF generation failed" | |
| }; | |
| } | |
| const sizeBytes = pdfResult.sizeBytes ?? pdfResult.pdfBuffer.length; | |
| return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, sizeBytes); |
| "document": { | ||
| "title": "Document details", | ||
| "type": "object", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require data in the document object.
The document object has no required array, so { document: {} } passes validation. A court list document without data is meaningless. Add "required": ["data"] to the document schema.
🔧 Proposed fix
"document": {
"title": "Document details",
"type": "object",
+ "required": ["data"],
"properties": {📝 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.
| "document": { | |
| "title": "Document details", | |
| "type": "object", | |
| "document": { | |
| "title": "Document details", | |
| "type": "object", | |
| "required": ["data"], |
| "room": { | ||
| "title": "Courtroom name", | ||
| "type": "integer" | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix room title/type mismatch.
The room field has title "Courtroom name" but type integer. The tests confirm integer is intentional (room: 1). Update the title to "Courtroom number" for clarity.
🔧 Proposed fix
"room": {
- "title": "Courtroom name",
+ "title": "Courtroom number",
"type": "integer"
},📝 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.
| "room": { | |
| "title": "Courtroom name", | |
| "type": "integer" | |
| }, | |
| "room": { | |
| "title": "Courtroom number", | |
| "type": "integer" | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8715a2-f4fb-4d28-a207-c4749af6d098
📒 Files selected for processing (4)
apps/web/src/pages/(list-types)/magistrates-public-adult-court-list/index.test.tsapps/web/src/pages/(list-types)/magistrates-public-adult-court-list/index.tsapps/web/src/pages/(list-types)/magistrates-public-adult-court-list/magistrates-public-adult-court-list.njklibs/list-types/common/src/rendering/date-formatting.ts
💤 Files with no reviewable changes (1)
- apps/web/src/pages/(list-types)/magistrates-public-adult-court-list/magistrates-public-adult-court-list.njk
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/list-types/common/src/rendering/date-formatting.ts
| contentDate: artefact.contentDate, | ||
| locale | ||
| }); | ||
| const dataSource = resolveDataSource(artefact.provenance); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass locale translations to resolveDataSource for Welsh provenance labels.
resolveDataSource accepts an optional t parameter with provenanceLabels for locale-specific overrides, but the controller doesn't pass it. If the en/cy locale objects define provenanceLabels, Welsh users will see English/default provenance labels instead of Welsh ones.
🛡️ Proposed fix
- const dataSource = resolveDataSource(artefact.provenance);
+ const dataSource = resolveDataSource(artefact.provenance, t);📝 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 dataSource = resolveDataSource(artefact.provenance); | |
| const dataSource = resolveDataSource(artefact.provenance, t); |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/list-types/upper-tribunal-administrative-appeals-chamber-daily-hearing-list/src/pdf/pdf-generator.ts (1)
11-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared type alias for the options parameter.
The
Parameters<ReturnType<typeof createUtDailyHearingListPdfGenerator<UtaacHearingList>>>[0]expression is correct but verbose, and the same pattern is repeated across all three wrapper files (UTAAC, UTLC, UTCC). A shared type alias exported fromupper-tribunal-commonwould improve readability and centralise the derivation.♻️ Proposed shared type alias in upper-tribunal-common
In
libs/list-types/upper-tribunal-common/src/pdf-generator.ts:+export type UtDailyHearingListPdfOptions<T> = Parameters<ReturnType<typeof createUtDailyHearingListPdfGenerator<T>>>[0];Then in each wrapper:
-export async function generateUtaacDailyHearingListPdf(options: Parameters<ReturnType<typeof createUtDailyHearingListPdfGenerator<UtaacHearingList>>>[0]) { +export async function generateUtaacDailyHearingListPdf(options: UtDailyHearingListPdfOptions<UtaacHearingList>) {Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 233bcf26-49fb-43e4-84ed-2c83eae68fe1
📒 Files selected for processing (4)
libs/list-types/magistrates-public-adult-court-list/src/locales/en.tslibs/list-types/upper-tribunal-administrative-appeals-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/list-types/upper-tribunal-lands-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/list-types/upper-tribunal-tax-and-chancery-chamber-daily-hearing-list/src/pdf/pdf-generator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/list-types/magistrates-public-adult-court-list/src/locales/en.ts
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Resolve conflicts in upper-tribunal pdf-generator files by adopting the master version: use const + createUtDailyHearingListPdfGenerator directly instead of the feature-branch wrapper function pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove id fields from MAGISTRATES_PUBLIC_ADULT_COURT_LIST_DAILY and _FUTURE entries — ListTypeData has no id property, causing build failures. Also assign subJurisdictionIds: [7] (Magistrates Court) to both, matching the pattern used by all other magistrates/SJP list types. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Resolved conflicts between feature branch (magistrates-public-adult-court-list) and master (magistrates-adult-court-list), keeping both list types with their respective list type names, PDF generators, email builders, and registry entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-List Keep both magistrates-adult-court-list and magistrates-public-adult-court-list list types — both packages exist and serve different use cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (7)
libs/list-types/magistrates-adult-court-list/src/rendering/renderer.ts (1)
4-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove interfaces and types to the end of the module.
The declarations currently precede the exported and private functions.
As per coding guidelines, “Order module contents with top-level constants first, exported functions next, other functions in usage order, and interfaces/types last.”
Source: Coding guidelines
libs/list-types/magistrates-adult-court-list/src/locales/en.ts (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing punctuation.
Consider adding a colon after "Warning" for clearer punctuation, assuming the text isn't separated by structural styling in the template.
♻️ Proposed refactor
restrictionInformationBoldText: - "Warning You'll be in contempt of court if you publish any information which is protected by a reporting restriction. You could get a fine, prison sentence or both.", + "Warning: You'll be in contempt of court if you publish any information which is protected by a reporting restriction. You could get a fine, prison sentence or both.",libs/list-types/magistrates-adult-court-list/src/locales/cy.ts (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove inconsistent trailing full stop.
This heading includes a trailing full stop, which is inconsistent with the English locale equivalent (
restrictionInformationHeading).♻️ Proposed refactor
- restrictionInformationHeading: "Cyfyngiadau ar gyhoeddi neu ysgrifennu am yr achosion hyn.", + restrictionInformationHeading: "Cyfyngiadau ar gyhoeddi neu ysgrifennu am yr achosion hyn",libs/list-types/magistrates-adult-court-list/src/pdf/pdf-generator.ts (2)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove interface to the end of the file.
As per coding guidelines, order module contents with interfaces and types last. Please relocate
PdfGenerationOptionsto the bottom of the file.Source: Coding guidelines
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a static import instead of a dynamic import.
Prefer a static top-level import for standard dependencies unless lazy loading is explicitly required to avoid circular dependencies.
♻️ Proposed refactor
At the top of the file, add:
import { PROVENANCE_LABELS } from "`@hmcts/publication`";And update this logic:
- const { PROVENANCE_LABELS } = await import("`@hmcts/publication`"); const provenanceLabel = options.provenance ? PROVENANCE_LABELS[options.provenance as keyof typeof PROVENANCE_LABELS] || options.provenance : "";docs/tickets/514/tasks.md (1)
22-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd integration coverage for registry and subscription fulfilment wiring.
The checklist tests the builders, but not that both list-type names resolve through the PDF and email registries or that subscription fulfilment invokes them. Add an integration test for each variant before marking the fulfilment work complete.
libs/list-types/magistrates-adult-court-list/src/validation/json-validator.test.ts (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse SCREAMING_SNAKE_CASE for top-level constants.
As per coding guidelines, constants declared outside function scope must use
SCREAMING_SNAKE_CASE. Please renamevalidMinimalDataandvalidFullDatatoVALID_MINIMAL_DATAandVALID_FULL_DATA.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13b87b30-89df-491a-9a7a-b6c22ff90f57
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (47)
.github/workflows/claude.ymlapps/web/package.jsonapps/web/src/app.tsapps/web/src/assets/css/list-types/magistrates-adult-court-list.scssapps/web/src/assets/css/web.scssapps/web/src/pages/(list-types)/magistrates-adult-court-list/index.test.tsapps/web/src/pages/(list-types)/magistrates-adult-court-list/index.tsapps/web/src/pages/(list-types)/magistrates-adult-court-list/magistrates-adult-court-list.njkdocs/tickets/514/plan.mddocs/tickets/514/tasks.mddocs/tickets/514/ticket.mdlibs/list-types/common/src/list-type-data.tslibs/list-types/common/src/validation/list-type-validator.test.tslibs/list-types/common/src/validation/list-type-validator.tslibs/list-types/magistrates-adult-court-list/package.jsonlibs/list-types/magistrates-adult-court-list/src/assets/css/magistrates-adult-court-list.scsslibs/list-types/magistrates-adult-court-list/src/config.tslibs/list-types/magistrates-adult-court-list/src/email-summary/summary-builder.test.tslibs/list-types/magistrates-adult-court-list/src/email-summary/summary-builder.tslibs/list-types/magistrates-adult-court-list/src/index.tslibs/list-types/magistrates-adult-court-list/src/locales/cy.tslibs/list-types/magistrates-adult-court-list/src/locales/en.tslibs/list-types/magistrates-adult-court-list/src/pdf/pdf-generator.test.tslibs/list-types/magistrates-adult-court-list/src/pdf/pdf-generator.tslibs/list-types/magistrates-adult-court-list/src/pdf/pdf-template.njklibs/list-types/magistrates-adult-court-list/src/rendering/renderer.test.tslibs/list-types/magistrates-adult-court-list/src/rendering/renderer.tslibs/list-types/magistrates-adult-court-list/src/schemas/magistrates-adult-court-list.jsonlibs/list-types/magistrates-adult-court-list/src/validation/json-validator.test.tslibs/list-types/magistrates-adult-court-list/src/validation/json-validator.tslibs/list-types/magistrates-adult-court-list/tsconfig.jsonlibs/list-types/upper-tribunal-administrative-appeals-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/list-types/upper-tribunal-lands-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/list-types/upper-tribunal-tax-and-chancery-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/notifications/package.jsonlibs/notifications/src/notification/notification-service.tslibs/publication/package.jsonlibs/publication/src/artefact-search-extractor.test.tslibs/publication/src/artefact-search-extractor.tslibs/publication/src/index.tslibs/publication/src/processing/service.tslibs/publication/src/repository/artefact-search-queries.test.tslibs/publication/src/repository/artefact-search-queries.tslibs/publication/src/repository/queries.test.tslibs/publication/src/repository/queries.tstsconfig.jsonturbo.json
💤 Files with no reviewable changes (1)
- libs/publication/src/repository/queries.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/publication/package.json
- apps/web/src/app.ts
- apps/web/package.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 20
🧹 Nitpick comments (7)
libs/list-types/magistrates-adult-court-list/src/rendering/renderer.ts (1)
4-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove interfaces and types to the end of the module.
The declarations currently precede the exported and private functions.
As per coding guidelines, “Order module contents with top-level constants first, exported functions next, other functions in usage order, and interfaces/types last.”
Source: Coding guidelines
libs/list-types/magistrates-adult-court-list/src/locales/en.ts (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing punctuation.
Consider adding a colon after "Warning" for clearer punctuation, assuming the text isn't separated by structural styling in the template.
♻️ Proposed refactor
restrictionInformationBoldText: - "Warning You'll be in contempt of court if you publish any information which is protected by a reporting restriction. You could get a fine, prison sentence or both.", + "Warning: You'll be in contempt of court if you publish any information which is protected by a reporting restriction. You could get a fine, prison sentence or both.",libs/list-types/magistrates-adult-court-list/src/locales/cy.ts (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove inconsistent trailing full stop.
This heading includes a trailing full stop, which is inconsistent with the English locale equivalent (
restrictionInformationHeading).♻️ Proposed refactor
- restrictionInformationHeading: "Cyfyngiadau ar gyhoeddi neu ysgrifennu am yr achosion hyn.", + restrictionInformationHeading: "Cyfyngiadau ar gyhoeddi neu ysgrifennu am yr achosion hyn",libs/list-types/magistrates-adult-court-list/src/pdf/pdf-generator.ts (2)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove interface to the end of the file.
As per coding guidelines, order module contents with interfaces and types last. Please relocate
PdfGenerationOptionsto the bottom of the file.Source: Coding guidelines
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a static import instead of a dynamic import.
Prefer a static top-level import for standard dependencies unless lazy loading is explicitly required to avoid circular dependencies.
♻️ Proposed refactor
At the top of the file, add:
import { PROVENANCE_LABELS } from "`@hmcts/publication`";And update this logic:
- const { PROVENANCE_LABELS } = await import("`@hmcts/publication`"); const provenanceLabel = options.provenance ? PROVENANCE_LABELS[options.provenance as keyof typeof PROVENANCE_LABELS] || options.provenance : "";docs/tickets/514/tasks.md (1)
22-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd integration coverage for registry and subscription fulfilment wiring.
The checklist tests the builders, but not that both list-type names resolve through the PDF and email registries or that subscription fulfilment invokes them. Add an integration test for each variant before marking the fulfilment work complete.
libs/list-types/magistrates-adult-court-list/src/validation/json-validator.test.ts (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse SCREAMING_SNAKE_CASE for top-level constants.
As per coding guidelines, constants declared outside function scope must use
SCREAMING_SNAKE_CASE. Please renamevalidMinimalDataandvalidFullDatatoVALID_MINIMAL_DATAandVALID_FULL_DATA.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13b87b30-89df-491a-9a7a-b6c22ff90f57
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (47)
.github/workflows/claude.ymlapps/web/package.jsonapps/web/src/app.tsapps/web/src/assets/css/list-types/magistrates-adult-court-list.scssapps/web/src/assets/css/web.scssapps/web/src/pages/(list-types)/magistrates-adult-court-list/index.test.tsapps/web/src/pages/(list-types)/magistrates-adult-court-list/index.tsapps/web/src/pages/(list-types)/magistrates-adult-court-list/magistrates-adult-court-list.njkdocs/tickets/514/plan.mddocs/tickets/514/tasks.mddocs/tickets/514/ticket.mdlibs/list-types/common/src/list-type-data.tslibs/list-types/common/src/validation/list-type-validator.test.tslibs/list-types/common/src/validation/list-type-validator.tslibs/list-types/magistrates-adult-court-list/package.jsonlibs/list-types/magistrates-adult-court-list/src/assets/css/magistrates-adult-court-list.scsslibs/list-types/magistrates-adult-court-list/src/config.tslibs/list-types/magistrates-adult-court-list/src/email-summary/summary-builder.test.tslibs/list-types/magistrates-adult-court-list/src/email-summary/summary-builder.tslibs/list-types/magistrates-adult-court-list/src/index.tslibs/list-types/magistrates-adult-court-list/src/locales/cy.tslibs/list-types/magistrates-adult-court-list/src/locales/en.tslibs/list-types/magistrates-adult-court-list/src/pdf/pdf-generator.test.tslibs/list-types/magistrates-adult-court-list/src/pdf/pdf-generator.tslibs/list-types/magistrates-adult-court-list/src/pdf/pdf-template.njklibs/list-types/magistrates-adult-court-list/src/rendering/renderer.test.tslibs/list-types/magistrates-adult-court-list/src/rendering/renderer.tslibs/list-types/magistrates-adult-court-list/src/schemas/magistrates-adult-court-list.jsonlibs/list-types/magistrates-adult-court-list/src/validation/json-validator.test.tslibs/list-types/magistrates-adult-court-list/src/validation/json-validator.tslibs/list-types/magistrates-adult-court-list/tsconfig.jsonlibs/list-types/upper-tribunal-administrative-appeals-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/list-types/upper-tribunal-lands-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/list-types/upper-tribunal-tax-and-chancery-chamber-daily-hearing-list/src/pdf/pdf-generator.tslibs/notifications/package.jsonlibs/notifications/src/notification/notification-service.tslibs/publication/package.jsonlibs/publication/src/artefact-search-extractor.test.tslibs/publication/src/artefact-search-extractor.tslibs/publication/src/index.tslibs/publication/src/processing/service.tslibs/publication/src/repository/artefact-search-queries.test.tslibs/publication/src/repository/artefact-search-queries.tslibs/publication/src/repository/queries.test.tslibs/publication/src/repository/queries.tstsconfig.jsonturbo.json
💤 Files with no reviewable changes (1)
- libs/publication/src/repository/queries.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/publication/package.json
- apps/web/src/app.ts
- apps/web/package.json
🛑 Comments failed to post (20)
apps/web/src/assets/css/list-types/magistrates-adult-court-list.scss (1)
1-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the Magistrates styles and retain one canonical implementation.
Both files expose list-specific overrides through generic GOV.UK selectors, allowing them to affect unrelated pages.
apps/web/src/assets/css/list-types/magistrates-adult-court-list.scss#L1-L37: nest the rules beneath a page-specific wrapper and import the canonical stylesheet.libs/list-types/magistrates-adult-court-list/src/assets/css/magistrates-adult-court-list.scss#L1-L37: retain the shared scoped rules here rather than duplicating them in the application.📍 Affects 2 files
apps/web/src/assets/css/list-types/magistrates-adult-court-list.scss#L1-L37(this comment)libs/list-types/magistrates-adult-court-list/src/assets/css/magistrates-adult-court-list.scss#L1-L37apps/web/src/assets/css/web.scss (1)
11-11: 📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Remove the
.scssextension from the@usepath.This triggers the configured
scss/load-partial-extensionStylelint error.-@use "./list-types/magistrates-adult-court-list.scss"; +@use "./list-types/magistrates-adult-court-list";📝 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.`@use` "./list-types/magistrates-adult-court-list";🧰 Tools
🪛 Stylelint (17.14.0)
[error] 11-11: Unexpected extension ".scss" in
@use(scss/load-partial-extension)(scss/load-partial-extension)
Source: Linters/SAST tools
apps/web/src/pages/(list-types)/magistrates-adult-court-list/index.test.ts (2)
35-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use arbitrary IDs and drive each case solely through
listTypeName.Values
57and58couple these tests to environment registry data. Use one arbitrary ID while varying onlylistTypeName.Proposed change
const LIST_TYPE_CASES = [ - { listTypeName: "MAGISTRATES_ADULT_COURT_LIST_DAILY", listTypeId: 57 }, - { listTypeName: "MAGISTRATES_ADULT_COURT_LIST_FUTURE", listTypeId: 58 } + { listTypeName: "MAGISTRATES_ADULT_COURT_LIST_DAILY", listTypeId: 12345 }, + { listTypeName: "MAGISTRATES_ADULT_COURT_LIST_FUTURE", listTypeId: 12345 } ];As per coding guidelines, “List-type test fixtures must use arbitrary numeric IDs and drive behavior through
listTypeName.”📝 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 LIST_TYPE_CASES = [ { listTypeName: "MAGISTRATES_ADULT_COURT_LIST_DAILY", listTypeId: 12345 }, { listTypeName: "MAGISTRATES_ADULT_COURT_LIST_FUTURE", listTypeId: 12345 } ];Source: Coding guidelines
53-53: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail FILE='apps/web/src/pages/(list-types)/magistrates-adult-court-list/index.test.ts' echo '--- file outline ---' ast-grep outline "$FILE" --view expanded || true echo '--- relevant lines ---' sed -n '1,260p' "$FILE" | cat -n echo '--- search for fixture/builders in related area ---' rg -n "builder|fixture|satisfies|as any|mockResolvedValue|listType.findUnique|renderer" "apps/web/src/pages/(list-types)/magistrates-adult-court-list" -g '!**/node_modules/**' || trueRepository: hmcts/cath-service
Length of output: 13770
🏁 Script executed:
set -euo pipefail echo '--- locate relevant source files ---' fd -a "magistrates-adult-court-list" apps libs | sed -n '1,120p' echo '--- search for exported types/functions ---' rg -n "validateMagistratesAdultCourtList|renderMagistratesAdultCourtList|type .*MagistratesAdultCourtList|interface .*MagistratesAdultCourtList|Artefact" apps libs -g '!**/node_modules/**' || trueRepository: hmcts/cath-service
Length of output: 50375
🏁 Script executed:
set -euo pipefail echo '--- renderer.ts types ---' sed -n '1,180p' libs/list-types/magistrates-adult-court-list/src/rendering/renderer.ts | cat -n echo '--- json-validator.ts types ---' sed -n '1,140p' libs/list-types/magistrates-adult-court-list/src/validation/json-validator.ts | cat -n echo '--- publication model types ---' sed -n '1,80p' libs/publication/src/repository/model.ts | cat -nRepository: hmcts/cath-service
Length of output: 7963
Replace the
as anycasts with typed fixtures orsatisfieschecks.
The same applies to the mockedvalidateMagistratesAdultCourtList,renderMagistratesAdultCourtList, andbuildMockArtefactreturn values in this test.Source: Coding guidelines
apps/web/src/pages/(list-types)/magistrates-adult-court-list/magistrates-adult-court-list.njk (2)
24-40: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the target template and nearby page files git ls-files 'apps/web/src/pages/(list-types)/magistrates-adult-court-list/*' | sort echo echo '--- outline ---' ast-grep outline 'apps/web/src/pages/(list-types)/magistrates-adult-court-list/magistrates-adult-court-list.njk' --view expanded || true echo echo '--- file with line numbers ---' cat -n 'apps/web/src/pages/(list-types)/magistrates-adult-court-list/magistrates-adult-court-list.njk' | sed -n '1,220p' echo echo '--- search for GOV.UK macros usage in this page ---' rg -n "macro|warning-text|accordion|table|govukTable|govukAccordion|govukWarningText" 'apps/web/src/pages/(list-types)/magistrates-adult-court-list' -SRepository: hmcts/cath-service
Length of output: 14247
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect shared macro definitions/usages that this page may rely on git ls-files 'apps/web/src/components/**' 'apps/web/src/macros/**' 'apps/web/src/lib/**' | rg 'accordion|table|warning|govuk' || true echo echo '--- locate macro calls in the repository ---' rg -n "{%\s*from|govukWarningText|govukAccordion|govukTable|warningText|accordion|table" apps/web/src -g '*.njk' -g '*.ts' -S | sed -n '1,220p'Repository: hmcts/cath-service
Length of output: 358
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the full template with line numbers, limited to a reasonable range sed -n '1,220p' 'apps/web/src/pages/(list-types)/magistrates-adult-court-list/magistrates-adult-court-list.njk' | cat -nRepository: hmcts/cath-service
Length of output: 6989
Use GOV.UK macros for the warning, accordion and table blocks.
These are hand-written here, so they bypass the maintained accessibility and behaviour from the GOV.UK component library.Source: Coding guidelines
44-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add an associated label for the case-search input.
The heading does not provide an accessible name for the input. Render a
<label for="case-search-input">using the localised search text.Proposed change
- <h2 class="govuk-heading-m">{{ t.searchCases }}</h2> + <label class="govuk-label govuk-label--m" for="case-search-input"> + {{ t.searchCases }} + </label> <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text">📝 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.<div class="govuk-form-group govuk-!-margin-top-6"> <label class="govuk-label govuk-label--m" for="case-search-input"> {{ t.searchCases }} </label> <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text"> </div>docs/tickets/514/plan.md (1)
49-51: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail printf '\n== plan excerpt ==\n' cat -n docs/tickets/514/plan.md | sed -n '35,70p' printf '\n== repo references ==\n' rg -n "magistrates[-_ ]adult[-_ ]court[-_ ]list|magistrates_adult_court_list|bstart|Draft 2020-12|Draft-07|Offence Code|Block Start" docs src .github -S || trueRepository: hmcts/cath-service
Length of output: 9783
🌐 Web query:
hmcts pip-data-management magistrates_adult_court_list.json Draft 2020-12 bstart offence code title summary required💡 Result:
The query appears to conflate several unrelated concepts within the HMCTS (His Majesty's Courts and Tribunals Service) domain and external technical or research terminology. 1. HMCTS PIP (Publication of Information for Public) Data Management: The pip-data-management and pip-data-models repositories are components of the HMCTS Court and Tribunal Hearings Service (CaTH) [1]. These services handle the persistence, validation, and retrieval of court publications, including the Magistrates Adult Court List [1]. The system uses JSON-based structures for data handling, and documentation (such as style guides) exists for these lists [2]. 2. "bstart" and Offence Codes: There is no evidence of an HMCTS "bstart" field related to offence codes. The term "Bstarting point" (or Bstart) appears extensively in academic research regarding the Cambridge Crime Harm Index (CCHI) [3][4]. In that context, "starting point" refers to the baseline days of imprisonment recommended by the Sentencing Council of England and Wales for a first-time offender, which researchers use to assign harm values to offence codes [5][3][4]. This is a research methodology, not an HMCTS data management standard. 3. "magistrates_adult_court_list.json" Draft 2020-12: While HMCTS uses JSON for court lists within the PIP service [1], there is no public record of a specific "Draft 2020-12" document for magistrates_adult_court_list.json. The current HMCTS model for magistrates court lists has evolved through various code changes [6][7][8][9] and is actively managed in the hmcts/pip-data-models repository [6]. 4. Other unrelated terms: The term "B.Start" is also a seed funding program in Malta [10], and "BSTART" is a buffer start command in specific hardware/software contexts [11], both of which are unrelated to HMCTS legal data.
Citations:
- 1: https://github.com/hmcts/pip-data-management/
- 2: #514
- 3: https://doi.org/10.1007/s41887-017-0016-9
- 4: https://doi.org/10.1007/s41887-017-0007-x
- 5: https://www.repository.cam.ac.uk/bitstreams/c91177c0-a875-4d74-8489-f754275452e6/download
- 6: hmcts/pip-data-models@033438b
- 7: hmcts/pip-data-models@62b355e
- 8: hmcts/pip-data-models@ac91aa1
- 9: hmcts/pip-data-models@88ee715
- 10: https://maltaenterprise.com/support/business-start-bstart
- 11: https://support.nipponpulse.com/NPA_Command_Reference/BSTARTISTART-BufferStart.html
🏁 Script executed:
set -euo pipefail printf '\n== ticket excerpt ==\n' cat -n docs/tickets/514/ticket.md | sed -n '24,40p' printf '\n== upstream schema (raw) ==\n' python3 - <<'PY' import urllib.request, ssl, sys url = 'https://raw.githubusercontent.com/hmcts/pip-data-management/master/src/main/resources/schemas/magistrates_adult_court_list.json' try: with urllib.request.urlopen(url, context=ssl.create_default_context(), timeout=20) as r: data = r.read().decode('utf-8') print(data[:6000]) except Exception as e: print(f'ERROR: {e}') sys.exit(1) PYRepository: hmcts/cath-service
Length of output: 1692
🏁 Script executed:
set -euo pipefail url='https://raw.githubusercontent.com/hmcts/pip-data-management/master/src/main/resources/schemas/magistrates_adult_court_list.json' printf '\n== upstream schema headers ==\n' curl -ksL "$url" | sed -n '1,220p' printf '\n== schema summary ==\n' curl -ksL "$url" | python3 - <<'PY' import json, sys data = json.load(sys.stdin) print('draft:', data.get('$schema')) print('top required:', data.get('required')) print('top properties:', list(data.get('properties', {}).keys())) defs = data.get('$defs') or data.get('definitions') or {} print('defs keys:', list(defs.keys())) for k, v in defs.items(): if isinstance(v, dict): if 'required' in v: print(f'{k} required:', v['required']) if 'properties' in v: print(f'{k} properties:', list(v['properties'].keys())) PYRepository: hmcts/cath-service
Length of output: 11229
Align this schema plan with the upstream contract. The referenced schema is Draft 2020-12 and uses the nested
document → data → job → sessions → session → blocks → block → cases → caseshape, withbstartrequired at block level andcaseno/def_namerequired per case, plus nested offence fields (code,title,sum). This plan currently describes a different Draft-07, flattened contract, so update the plan and validation tests or record an explicit schema fork.docs/tickets/514/tasks.md (1)
10-10: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not mark Welsh localisation complete while gaps remain.
docs/tickets/514/plan.mdexplicitly lists unresolved translations for Date of Birth, Address, Age, Offence Code, Offence Title and Offence Summary. Either add and approve those translations with tests, or leave this task incomplete.docs/tickets/514/ticket.md (2)
38-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the Welsh Daily title.
The English title is “Magistrates Adult Court List - Daily”, but the Welsh translation adds “Cyhoeddus” (“Public”), while the Future title does not. Confirm the intended product name and keep the English and Welsh labels semantically aligned.
111-135: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one authoritative list-type ID allocation across the ticket documents.
The ticket specifies 28/29, while the plan and task checklist specify 57/58. This must be resolved before implementation because the IDs flow into seed data and downstream registry resolution.
docs/tickets/514/ticket.md#L111-L135: confirm or replace IDs 28 and 29.docs/tickets/514/plan.md#L138-L145: update the central-wiring table to the agreed IDs.docs/tickets/514/tasks.md#L15-L15: update the completed seed-data task to the same allocation.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 111-111: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
📍 Affects 3 files
docs/tickets/514/ticket.md#L111-L135(this comment)docs/tickets/514/plan.md#L138-L145docs/tickets/514/tasks.md#L15-L15libs/list-types/common/src/validation/list-type-validator.ts (1)
26-27: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add missing aliases for the public variants.
The aliases for the public list types (
MAGISTRATES_PUBLIC_ADULT_COURT_LIST_DAILYandMAGISTRATES_PUBLIC_ADULT_COURT_LIST_FUTURE) are missing from this map.As defined in the test file, these list types are handled by the
@hmcts/magistrates-public-adult-court-listpackage. Without these explicit aliases,convertListTypeNameToKebabCasewill produce package names with the-dailyand-futuresuffixes (e.g.@hmcts/magistrates-public-adult-court-list-daily), causing dynamic imports and subsequent validations to fail.🐛 Proposed fix to include public aliases
"magistrates-adult-court-list-daily": "magistrates-adult-court-list", "magistrates-adult-court-list-future": "magistrates-adult-court-list", + "magistrates-public-adult-court-list-daily": "magistrates-public-adult-court-list", + "magistrates-public-adult-court-list-future": "magistrates-public-adult-court-list",📝 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."magistrates-adult-court-list-daily": "magistrates-adult-court-list", "magistrates-adult-court-list-future": "magistrates-adult-court-list", "magistrates-public-adult-court-list-daily": "magistrates-public-adult-court-list", "magistrates-public-adult-court-list-future": "magistrates-public-adult-court-list",libs/list-types/magistrates-adult-court-list/src/config.ts (1)
7-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n-- file outline --\n' ast-grep outline libs/list-types/magistrates-adult-court-list/src/config.ts --view expanded || true printf '\n-- config.ts with line numbers --\n' cat -n libs/list-types/magistrates-adult-court-list/src/config.ts printf '\n-- nearby tree --\n' find libs/list-types/magistrates-adult-court-list -maxdepth 4 -type f | sort | sed 's#^\./##' printf '\n-- search for assets references --\n' rg -n "magistrates-adult-court-list|src/assets|../assets/|assets/" libs/list-types/magistrates-adult-court-list -g '!**/dist/**' || trueRepository: hmcts/cath-service
Length of output: 2964
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n-- config consumers --\n' rg -n "\b(moduleRoot|assets|schemaPath)\b" libs/list-types/magistrates-adult-court-list/src -g '!**/*.test.ts' || true printf '\n-- package.json --\n' cat -n libs/list-types/magistrates-adult-court-list/package.json printf '\n-- tsconfig.json --\n' cat -n libs/list-types/magistrates-adult-court-list/tsconfig.json printf '\n-- renderer and pdf generator slices --\n' sed -n '1,220p' libs/list-types/magistrates-adult-court-list/src/rendering/renderer.ts printf '\n---\n' sed -n '1,260p' libs/list-types/magistrates-adult-court-list/src/pdf/pdf-generator.tsRepository: hmcts/cath-service
Length of output: 10248
Use SCREAMING_SNAKE_CASE and fix the assets path
moduleRoot,assets, andschemaPathare exported constants, so they should beSCREAMING_SNAKE_CASE.assetsalso points outsidesrc/; if this is meant to loadsrc/assets/, the path and build copy step need to line up.Source: Coding guidelines
libs/list-types/magistrates-adult-court-list/src/email-summary/summary-builder.ts (1)
6-22: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localise the email summary.
The builder has no locale input, hardcodes all labels in English and ignores
cy_title. Accept the locale/translations and use Welsh offence titles when requested.As per coding guidelines, “Court names and other list-type-specific display strings must come from English and Welsh locale files, not hardcoded controller strings.”
Source: Coding guidelines
libs/list-types/magistrates-adult-court-list/src/pdf/pdf-template.njk (1)
2-2: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set the PDF document language from the selected locale.
Welsh PDFs are currently declared as English. Pass
options.localeinto the template context and render it in thelangattribute.Proposed fix
-<html lang="en"> +<html lang="{{ locale }}">Also add
locale: options.localeto the template data inpdf-generator.ts.libs/list-types/magistrates-adult-court-list/src/rendering/renderer.test.ts (1)
10-10: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the unjustified
anycasts from these test fixtures and helpers.
libs/list-types/magistrates-adult-court-list/src/rendering/renderer.test.ts#L10-L10: derive the session fixture type fromMagistratesAdultCourtListData.libs/list-types/magistrates-adult-court-list/src/rendering/renderer.test.ts#L281-L286: derive the result type fromAwaited<ReturnType<typeof renderMagistratesAdultCourtList>>.libs/list-types/magistrates-adult-court-list/src/email-summary/summary-builder.test.ts#L10-L10: type the sessions array from the production data contract.As per coding guidelines, “TypeScript must use strict mode and must not use
anywithout justification.”📍 Affects 2 files
libs/list-types/magistrates-adult-court-list/src/rendering/renderer.test.ts#L10-L10(this comment)libs/list-types/magistrates-adult-court-list/src/rendering/renderer.test.ts#L281-L286libs/list-types/magistrates-adult-court-list/src/email-summary/summary-builder.test.ts#L10-L10Source: Coding guidelines
libs/list-types/magistrates-adult-court-list/src/rendering/renderer.ts (1)
145-153: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not turn a missing courtroom into
0.Line 151 fabricates “Courtroom 0” when
roomis absent, and the PDF template renders it unconditionally. Preserve the missing value and suppress the courtroom line when it is undefined.libs/list-types/magistrates-adult-court-list/src/schemas/magistrates-adult-court-list.json (1)
94-108: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Define missing properties to enable HTML injection validation.
The schema is missing definitions for several properties inside the
caseobject, includingdef_addr,inf,offences,def_dob, anddef_age. Because they are omitted andadditionalPropertiesis not restricted, Ajv will silently ignore them during validation.Consequently, the HTML injection patterns won't be applied to these fields, and the corresponding unit tests in
json-validator.test.tswill fail becauseisValidwill erroneously returntrue.Please define these properties with the appropriate
patternconstraints to ensure the payloads are securely validated. Do you want me to generate the complete schema patch?libs/list-types/magistrates-adult-court-list/src/validation/json-validator.ts (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add
@hmcts/publicationtopackage.jsondependencies.The
@hmcts/publicationmodule is imported here but is not listed as a dependency in thelibs/list-types/magistrates-adult-court-list/package.jsonmanifest. Please ensure it is added to prevent module resolution errors.libs/notifications/package.json (1)
39-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add missing dependency for
@hmcts/magistrates-public-adult-court-list.The
notification-service.tsfile imports functions from@hmcts/magistrates-public-adult-court-list, but it has not been added to the dependencies here alongside the non-public variant.🔧 Proposed fix
"`@hmcts/london-administrative-court-daily-cause-list`": "workspace:*", "`@hmcts/magistrates-adult-court-list`": "workspace:*", + "`@hmcts/magistrates-public-adult-court-list`": "workspace:*", "`@hmcts/magistrates-public-list`": "workspace:*",📝 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."`@hmcts/london-administrative-court-daily-cause-list`": "workspace:*", "`@hmcts/magistrates-adult-court-list`": "workspace:*", "`@hmcts/magistrates-public-adult-court-list`": "workspace:*", "`@hmcts/magistrates-public-list`": "workspace:*",libs/publication/src/repository/queries.test.ts (1)
2-5: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove duplicate tests and their corresponding import.
The tests for
createArtefactSearch,findArtefactSearchByArtefactId, anddeleteArtefactSearchByArtefactIdhave been fully migrated toartefact-search-queries.test.ts. Please remove this import, as well as the duplicate test blocks left at the bottom of this file (lines 1350-1549) and the now-unusedartefactSearchPrisma mock (lines 36-40).♻️ Proposed fix
-import { createArtefactSearch, deleteArtefactSearchByArtefactId, findArtefactSearchByArtefactId } from "./artefact-search-queries.js"; import { createArtefact,📝 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.import { createArtefact, deleteArtefacts,
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
2 participants
Jira link
#790
Change description
Add Magistrates Public Adult Court List style guide and PDF
Testing done
Security Vulnerability Assessment
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist
Summary by CodeRabbit