Feature/760 Update RPT lists and add new Market Rents list - #902
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the Market Rents weekly hearing list, updates RPT naming and locales, substitutes regional email addresses, conditionally renders Market Rents information, and wires the list through web, PDF, conversion, and notification paths. ChangesRPT Market Rents list
Sequence Diagram(s)sequenceDiagram
participant Publisher
participant PublicationProcessing
participant FttRptPdfGenerator
participant WebHandler
participant HearingListTemplate
Publisher->>PublicationProcessing: publish Market Rents hearing list
PublicationProcessing->>FttRptPdfGenerator: generate PDF with listTypeName
FttRptPdfGenerator->>HearingListTemplate: render regional email and conditional extra information
WebHandler->>HearingListTemplate: render public hearing list with resolved regional text
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright E2E Test Results82 tests 52 ✅ 6m 12s ⏱️ Results for commit 8a1d09b. ♻️ 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: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: baf616a2-0451-4d41-beab-0121f23f6ac9
📒 Files selected for processing (24)
apps/web/src/pages/(list-types)/ftt-rpt-weekly-hearing-list/ftt-rpt-weekly-hearing-list.njkapps/web/src/pages/(list-types)/ftt-rpt-weekly-hearing-list/ftt-rpt-weekly-hearing-list.njk.test.tsapps/web/src/pages/(list-types)/ftt-rpt-weekly-hearing-list/index.test.tsapps/web/src/pages/(list-types)/ftt-rpt-weekly-hearing-list/index.tsdocs/tickets/760/plan.mddocs/tickets/760/tasks.mddocs/tickets/760/ticket.mdlibs/list-types/common/src/list-type-data.tslibs/list-types/common/src/pdf/pdf-utilities.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/conversion/ftt-rpt-config.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/index.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/locales/cy.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/locales/en.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/locales/important-information-text.test.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/locales/important-information-text.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/locales/regional-email-guard.test.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/pdf/pdf-generator.test.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/pdf/pdf-generator.tslibs/list-types/ftt-rpt-weekly-hearing-list/src/pdf/pdf-template.njklibs/list-types/ftt-rpt-weekly-hearing-list/src/views/ftt-rpt-weekly-hearing-list.njklibs/location/src/location-data.tslibs/notifications/src/notification/notification-service.tslibs/public-pages/src/flat-file/flat-file-service.tslibs/publication/src/processing/service.ts
💤 Files with no reviewable changes (2)
- libs/list-types/ftt-rpt-weekly-hearing-list/src/views/ftt-rpt-weekly-hearing-list.njk
- libs/public-pages/src/flat-file/flat-file-service.ts
| it("should resolve the Welsh regional email and extra information text for Market Rents", async () => { | ||
| // Arrange | ||
| res.locals = { locale: "cy" }; | ||
| const mockArtefact = { | ||
| artefactId: "test-market-rents-cy", | ||
| listTypeId: 38, | ||
| listTypeName: "FTT_RPT_MARKET_RENTS_WEEKLY_HEARING_LIST", | ||
| contentDate: new Date("2026-01-01"), | ||
| lastReceivedDate: new Date("2026-01-01T12:00:00Z"), | ||
| provenance: "MANUAL_UPLOAD" | ||
| }; | ||
| req.query = { artefactId: "test-market-rents-cy" }; | ||
| vi.mocked(getArtefactById).mockResolvedValue(mockArtefact as any); | ||
| vi.mocked(getPublicationJson).mockResolvedValue(MOCK_JSON_DATA); | ||
| mockValidate.mockReturnValue({ isValid: true, errors: [] }); | ||
| vi.mocked(renderFttRptData).mockReturnValue({ | ||
| header: { listTitle: "title", weekCommencingDate: "", lastUpdatedDate: "", lastUpdatedTime: "" }, | ||
| hearings: [] | ||
| }); | ||
|
|
||
| // Act | ||
| await GET(req as Request, res as Response); | ||
|
|
||
| // Assert | ||
| expect(res.render).toHaveBeenCalledWith( | ||
| "ftt-rpt-weekly-hearing-list", | ||
| expect.objectContaining({ | ||
| importantInformationText: expect.stringContaining("marketrents@justice.gov.uk"), | ||
| extraInformationText: MARKET_RENTS_EXTRA_INFORMATION_CY | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test Welsh content through ?lng=cy.
This injects res.locals.locale directly, bypassing the i18n selection path. Add a request-level page test that exercises the route with ?lng=cy.
As per coding guidelines, “Test Welsh page content using the ?lng=cy query parameter.”
Source: Coding guidelines
| | `searchCasesLabel` | **No verified match anywhere in the codebase** (RPT's exact English wording is unique) | `[WELSH TRANSLATION REQUIRED: 'Search by case reference number, date, venue, or other details']` | | ||
| | `tableHeaders.date/time/venue/caseType/judges/members/hearingMethod` | **No verified match anywhere in the codebase** — even `grc-weekly-hearing-list` (which shares `judges`/`members` keys) has these marked `[WELSH TRANSLATION REQUIRED: ...]`, unresolved | `[WELSH TRANSLATION REQUIRED: '<English value>']` for each, following the exact bracket convention already used in `grc-weekly-hearing-list/src/locales/cy.ts` | | ||
| | `tableHeaders.caseReferenceNumber` | **No verified match** | `[WELSH TRANSLATION REQUIRED: 'Case reference number']` | | ||
| | `tableHeaders.additionalInformation` | Reused verbatim (verified in multiple sibling files) | "Gwybodaeth ychwanegol" | | ||
| | `rptEastern/London/Midlands/Northern/SouthernCourtName` + `PageTitle` (5 existing, untouched per Clarification 1) | **No verified match; ticket does not supply these** | `[WELSH TRANSLATION REQUIRED: '<English value>']` each | | ||
| | `rptMarketRentsCourtName` / `rptMarketRentsPageTitle` | **Ticket-supplied** | Ticket's Welsh translations for the new list title/short name | | ||
| | Venue name (`location-data.ts` `welshName`) | **Ticket-supplied** | "Tribiwnlys Haen Gyntaf (Siambr Eiddo) – Adran Eiddo Preswyl" | | ||
|
|
||
| **Rule applied:** never invent legal/procedural Welsh translations. Reuse only strings already verified as professionally translated identically elsewhere in the codebase; everything else uses the existing `[WELSH TRANSLATION REQUIRED: '...']` convention (already established in-repo, e.g. `grc-weekly-hearing-list/src/locales/cy.ts`) pending sign-off from the ticket's named Welsh translator (Kimberley Newton, per the ticket). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not release public Welsh placeholder text.
The plan and completed task record permit literal [WELSH TRANSLATION REQUIRED: ...] values for page content. That fails the Welsh-content acceptance criteria and would expose unfinished text to citizens. Obtain approved translations and keep this work unresolved until they are applied.
docs/tickets/760/plan.md#L167-L175: replace the placeholder strategy with approved Welsh content before declaring the implementation complete.docs/tickets/760/tasks.md#L11-L12: reopen the task until every public locale key has approved Welsh text.
📍 Affects 2 files
docs/tickets/760/plan.md#L167-L175(this comment)docs/tickets/760/tasks.md#L11-L12
| contentDate: Date; | ||
| courtName: string; | ||
| listTitle: string; | ||
| listTypeName?: string; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Require a configured list type for PDF generation.
An omitted or unmapped listTypeName silently produces an empty important-information paragraph. Require the stable list type name and fail configuration without a regional email instead of publishing a list without contact details. As per coding guidelines, use stable listTypeName for guards, routing, PDF generation, converter registration, and queries; never use numeric listTypeId or ListType.id.
Also applies to: 29-43
Source: Coding guidelines
| listTypeName?: string; | ||
| } | ||
|
|
||
| interface RptTranslations { | ||
| importantInformationTextTemplate: string; | ||
| rptRegionalEmail: Record<string, string>; | ||
| marketRentsExtraInformation: string; | ||
| } | ||
|
|
||
| export async function generateFttRptWeeklyHearingListPdf(options: PdfGenerationOptions): Promise<PdfGenerationResult> { | ||
| const listTypeName = options.listTypeName ?? ""; | ||
|
|
||
| return generateFttSiacWeeklyHearingListPdf({ | ||
| ...options, | ||
| moduleDir: __dirname, | ||
| provenanceLabel: options.provenance ? PROVENANCE_LABELS[options.provenance as keyof typeof PROVENANCE_LABELS] || options.provenance : "", | ||
| importEn: () => import("../locales/en.js"), | ||
| importCy: () => import("../locales/cy.js"), | ||
| generatePdf: generatePdfFromHtml, | ||
| renderData: renderFttRptData | ||
| renderData: renderFttRptData, | ||
| resolveTemplateVars: (translations) => { | ||
| const t = translations as unknown as RptTranslations; | ||
| const regionalEmail = t.rptRegionalEmail[listTypeName]; | ||
| const importantInformationText = regionalEmail ? buildImportantInformationText(t.importantInformationTextTemplate, regionalEmail) : ""; | ||
| const extraInformationText = listTypeName === MARKET_RENTS_LIST_TYPE ? t.marketRentsExtraInformation : ""; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make listTypeName mandatory throughout the RPT PDF flow. The optional fallback allows publication of PDFs with no regional contact details and no Market Rents guidance.
libs/list-types/ftt-rpt-weekly-hearing-list/src/pdf/pdf-generator.ts#L19-L43: requirelistTypeNameand reject unmapped regional-email configuration.libs/list-types/ftt-rpt-weekly-hearing-list/src/pdf/pdf-generator.test.ts#L227-L250: replace the successful missing-name scenario with a required-name/configuration-failure assertion.libs/publication/src/processing/service.ts#L256-L262: explicitly passFTT_RPT_MARKET_RENTS_WEEKLY_HEARING_LISTto the generator.
📍 Affects 3 files
libs/list-types/ftt-rpt-weekly-hearing-list/src/pdf/pdf-generator.ts#L19-L43(this comment)libs/list-types/ftt-rpt-weekly-hearing-list/src/pdf/pdf-generator.test.ts#L227-L250libs/publication/src/processing/service.ts#L256-L262
Source: Coding guidelines
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. |
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. |
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
2 participants
Jira link
#760
Change description
Update RPT lists and add new Market Rents list
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