VIBE-300 Add subscription by case name and case number - #318
Conversation
…ion and subscription process - Created specification document outlining database schema changes, admin configuration page, and publication processing requirements - Created implementation plan with phased approach and critical files to create/modify - Created detailed task list with clear dependencies and testing requirements Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…system This commit implements the database and application changes required to support multiple subscription search types (not just location-based). ## Database Changes: - Added `list_search_config` table to store JSON field mappings per list type - Added `artefact_search` table to store extracted case data from publications - Updated `subscription` table with `search_type` and `search_value` columns - Created migration script to migrate existing location subscriptions - Kept `location_id` column nullable for backwards compatibility ## New Module: list-search-config - Created admin configuration page at `/system-admin/list-configuration/:listTypeId/search-config` - Implemented repository and service layers with field name validation - Added bilingual support (English/Welsh) for all content - Includes form validation for field names (letters, numbers, underscores only) ## Publication Processing: - Created `artefact-search-extractor` to extract case data from JSON publications - Implemented `artefact-search-repository` for storing extracted data - Added graceful error handling for missing configurations or JSON fields ## Subscription Updates: - Updated subscription queries to use `search_type='LOCATION_ID'` and `search_value` - Modified subscription creation to populate new fields - Maintained backwards compatibility with existing location-based fulfilment ## Testing: - Added unit tests for list-search-config service validation logic - All builds passing without type errors - Linter checks passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…factor-artefact-search-extraction-subscription # Conflicts: # libs/publication/src/index.ts
Fixed type errors caused by making subscription.locationId nullable: - Updated SubscriptionWithUser interface in notifications to include new fields - Updated subscription queries to use searchType/searchValue - Updated mapSubscriptionToDto to handle nullable locationId and location - Added null filtering to subscription mapping functions - Fixed artefact search extractor type annotations All builds now passing (24/24 successful). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Completed the final migration to fully remove locationId from the subscription system, consolidating to searchType/searchValue only. ## Database Changes: - Dropped location_id column from subscription table - Dropped location_id foreign key constraint - Dropped unique_user_location constraint - Dropped idx_subscription_location index - Added unique_user_subscription constraint on (user_id, search_type, search_value) - Made search_type and search_value NOT NULL ## Code Changes: **Subscription Module:** - Updated queries to use searchType/searchValue for duplicate detection - Removed locationId from createSubscriptionRecord - Removed location includes from subscription queries - Updated service to fetch location data separately via getLocationById - Updated replaceUserSubscriptions to extract locationId from searchValue **Location Module:** - Removed subscriptions relation from Location model - Updated hasActiveSubscriptions to use searchType/searchValue **Notifications Module:** - Updated SubscriptionWithUser interface (removed nullable types) - Already using searchType/searchValue queries All builds passing (24/24 successful). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added list-search-config routes to the web application. The page is now accessible at: - GET/POST /list-search-config/:listTypeId Requires SYSTEM_ADMIN role to access. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Renamed page files to use Express parameter syntax [listTypeId] instead of :listTypeId to work with the simple-router file discovery. The route is now accessible at: - GET/POST /list-search-config/:listTypeId Changes: - Moved files to list-search-config/ subdirectory - Renamed files to use [listTypeId] syntax for route params - Updated import paths to reference service from parent directory - Updated template name to include subdirectory path 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changed template name from [listTypeId].njk to config.njk since Nunjucks looks for literal filenames and can't resolve brackets. The route file [listTypeId].ts still uses bracket syntax for Express route parameters, but references the template as 'list-search-config/config'.
Added moduleRoot export to list-search-config config and registered it in the web app's modulePaths array so Nunjucks can find the templates. This fixes the 'template not found' error when accessing the page. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed template to use the correct base template name and block: - Changed extends from 'base-templates.njk' to 'base-template.njk' (singular) - Changed block from 'content' to 'page_content' to match the layout 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changed POST success redirect from '/system-admin/list-configuration' (which doesn't exist) to the same config page so users see their saved configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added action="" to the form element to ensure proper form submission. Without an explicit action, some browsers may not submit the form correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added type='submit' attribute to the govukButton to ensure it triggers form submission. Without this, the button might default to type='button' which doesn't submit forms. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The govukButton macro was causing the form to close prematurely, placing the button outside the form tag. Replaced with a plain HTML button element that has the same GOV.UK styling classes. Also improved errorMessage conditionals to check both fieldErrors existence and the specific field error. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request implements a multi-search-type subscription system, refactoring the location-based subscription model to support searchable subscriptions by case name, case reference, and location. It introduces list-search-config infrastructure for configurable field extraction from publication data, case search functionality, and new UI flows for subscription management and administration across database, library modules, pages, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SearchPage as Case Search<br/>(UI)
participant SearchService as Case Search<br/>Service
participant Repository as Artefact Search<br/>Repository
participant Database as Database
participant PendingPage as Pending<br/>Subscriptions (UI)
participant SubService as Subscription<br/>Service
participant NotifyService as Notification<br/>Service
User->>SearchPage: Enter case name/reference
SearchPage->>SearchService: searchByCaseName(input)
SearchService->>Repository: findByCaseName/findByCaseNumber
Repository->>Database: Query artefact_search
Database-->>Repository: Case results
Repository-->>SearchService: CaseSearchResult[]
SearchService-->>SearchPage: Return deduplicated results
SearchPage-->>User: Display matching cases
User->>SearchPage: Select case(s) to subscribe
SearchPage->>PendingPage: POST selected cases (searchType: CASE_NAME)
PendingPage->>User: Show pending subscriptions
User->>PendingPage: Confirm subscriptions
PendingPage->>SubService: createCaseSubscription(userId, searchType, caseNumber, caseName)
SubService->>Database: INSERT into subscription (searchType, searchValue, caseNumber, caseName)
Database-->>SubService: Subscription created
SubService-->>PendingPage: Success
PendingPage->>NotifyService: New case subscription created
NotifyService->>Repository: findByCaseNumber for artefact data
Repository->>Database: Query artefact_search
Database-->>Repository: Artefact case data
NotifyService->>NotifyService: Extract case info, build template
NotifyService-->>User: Email notification sent
PendingPage-->>User: Confirmation page with case details
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 Results255 tests 233 ✅ 58m 51s ⏱️ For more details on these failures, see this check. Results for commit 630d3ef. ♻️ 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: 3
Note
Due to the large number of review comments, Critical 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 (5)
libs/system-admin-pages/src/pages/system-admin-dashboard/cy.ts (1)
24-28: Remove Welsh localization from admin dashboard.The Welsh locale file is imported and used here, but per the established pattern, Welsh localization is not required for admin screens. The href inconsistency between
/blob-explorer-locations(en.ts) and/blob-explorer(cy.ts) is a symptom of this—remove the cy.ts import and the conditional language loading logic entirely.libs/notifications/src/notification/notification-service.test.ts (1)
20-291: Add test coverage for case-based subscriptions in the notification service.The main service implementation handles both case number and case name subscriptions (including deduplication), but the test file only covers location-based scenarios. Add tests for:
- Sending notifications to case number subscribers
- Sending notifications to case name subscribers
- Deduplication when a user has both location and case subscriptions
libs/verified-pages/src/pages/bulk-unsubscribe/index.njk (1)
1-1: Use the required base layout name.This template extends
layouts/base-template.njk, but the required base islayouts/base-templates.njk.Proposed fix
-{% extends "layouts/base-template.njk" %} +{% extends "layouts/base-templates.njk" %}As per coding guidelines.
libs/notifications/src/notification/subscription-queries.test.ts (1)
101-117: Test expectation mismatches Prismaselectin implementation.
findActiveSubscriptionsByCaseNumbersusesselect(seelibs/notifications/src/notification/subscription-queries.tsLines 42–72), but the test expectsinclude. This will fail. Update the expectation to match the actual query shape.✅ Proposed fix
expect(prisma.subscription.findMany).toHaveBeenCalledWith({ where: { searchType: "CASE_NUMBER", searchValue: { in: ["CASE-123", "CASE-456"] } }, - include: { - user: { - select: { - email: true, - firstName: true, - surname: true - } - } - } + select: { + subscriptionId: true, + userId: true, + searchType: true, + searchValue: true, + caseName: true, + caseNumber: true, + user: { + select: { + email: true, + firstName: true, + surname: true + } + } + } });libs/subscription/src/repository/service.test.ts (1)
394-400: Stale test: case subscriptions are now implemented.This test asserts "case subscriptions are not yet implemented" but lines 763-819 test the actual implementation. Update or remove this placeholder test.
Suggested fix
Remove the placeholder test block (lines 394-400) since the actual implementation tests exist at lines 763-819.
♻️ Duplicate comments (1)
apps/postgres/prisma/migrations/20260123151728_add_cascade_delete_to_notification_audit_log/migration.sql (1)
1-5: Same retention concern as the Prisma schema change.See the earlier comment on cascading deletes and audit‑log retention.
🟠 Major comments (21)
libs/list-search-config/src/config.ts-1-8 (1)
1-8: Missing standard config exports for module registration.
config.tsshould exportpageRoutes,apiRoutes,prismaSchemas, andassets. OnlyprismaSchemasis present, which can break the standard module loader expectations.Proposed minimal fix
import path from "node:path"; import { fileURLToPath } from "node:url"; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); -// Module configuration for app registration -export const prismaSchemas = path.join(__dirname, "../prisma"); +// Module configuration for app registration +export const pageRoutes = []; +export const apiRoutes = []; +export const assets = []; +export const prismaSchemas = path.join(__dirname, "../prisma");As per coding guidelines, module config files must export the standard interfaces.
libs/verified-pages/src/pages/case-number-search-results/index.njk-1-1 (1)
1-1: Use the required base layout filename.
Guidelines specifylayouts/base-templates.njk(plural). Please align to prevent template resolution issues. As per coding guidelines, ...🔧 Proposed fix
-{% extends "layouts/base-template.njk" %} +{% extends "layouts/base-templates.njk" %}libs/verified-pages/src/pages/case-name-search/index.njk-1-1 (1)
1-1: Use the required base layout filename.
Guidelines requirelayouts/base-templates.njk(plural). Please align to avoid template lookup failures. As per coding guidelines, ...🔧 Proposed fix
-{% extends "layouts/base-template.njk" %} +{% extends "layouts/base-templates.njk" %}libs/verified-pages/src/pages/case-number-search-results/index.njk-2-32 (1)
2-32: Prefer GOV.UK table macro for compliance.
The table is rendered with raw HTML; guidelines require GOV.UK component macros. Consider switching togovukTable. As per coding guidelines, ...♻️ Proposed refactor
-{% from "govuk/components/button/macro.njk" import govukButton %} +{% from "govuk/components/button/macro.njk" import govukButton %} +{% from "govuk/components/table/macro.njk" import govukTable %} @@ - <table class="govuk-table"> - <thead class="govuk-table__head"> - <tr class="govuk-table__row"> - <th scope="col" class="govuk-table__header">{{ tableHeaderCaseName }}</th> - <th scope="col" class="govuk-table__header govuk-table__header--numeric">{{ tableHeaderCaseNumber }}</th> - </tr> - </thead> - <tbody class="govuk-table__body"> - {% for result in results %} - <tr class="govuk-table__row"> - <td class="govuk-table__cell">{{ result.caseName or notAvailable }}</td> - <td class="govuk-table__cell govuk-table__cell--numeric">{{ result.caseNumber or notAvailable }}</td> - </tr> - {% endfor %} - </tbody> - </table> + {% set rows = [] %} + {% for result in results %} + {% set rows = rows.concat([[{ text: result.caseName or notAvailable }, { text: result.caseNumber or notAvailable }]]) %} + {% endfor %} + {{ govukTable({ + head: [ + { text: tableHeaderCaseName }, + { text: tableHeaderCaseNumber, classes: "govuk-table__header--numeric" } + ], + rows: rows + }) }}e2e-tests/tests/case-subscriptions.spec.ts-15-31 (1)
15-31: Avoid hard-codedlocationId: 1to prevent brittle test data. This assumes seed data stability across environments and can break CI. Prefer deriving a location ID from configuration or an existing DB record.🛠️ Example approach using an env var
async function createTestCaseData(): Promise<TestCaseData> { const timestamp = Date.now(); const randomId = Math.floor(Math.random() * 1000000); + const locationId = Number(process.env.TEST_LOCATION_ID); + if (!Number.isInteger(locationId)) { + throw new Error("TEST_LOCATION_ID must be set for e2e data seeding"); + } const testCaseName = `Test Case ${timestamp}-${randomId}`; const testCaseNumber = `TC-${timestamp}-${randomId}`; const testArtefactId = `test-artefact-${timestamp}-${randomId}`; @@ await prisma.artefactSearch.create({ data: { artefactId: testArtefactId, caseName: testCaseName, caseNumber: testCaseNumber, - locationId: 1 // Use first location from seed data + locationId } });libs/api/src/blob-ingestion/repository/service.ts-85-86 (1)
85-86: Prevent case data appearing in logs during extraction.Lines 85-86 invoke
extractAndStoreArtefactSearch, which currently logs sample case numbers/names (seelibs/publication/src/artefact-search-extractor.ts, Lines 151‑161). That is likely sensitive; please redact or remove those fields (or gate behind debug-only logging).🔒 Suggested redaction (in
libs/publication/src/artefact-search-extractor.ts)- console.log(`[ArtefactSearch] Extracted ${cases.length} cases for artefact ${artefactId}:`, { - caseSamples: cases.slice(0, 5).map((c) => ({ caseNumber: c.caseNumber, caseName: c.caseName })) - }); + console.log(`[ArtefactSearch] Extracted ${cases.length} cases for artefact ${artefactId}`);As per coding guidelines, avoid logging sensitive data.
libs/verified-pages/src/pages/pending-subscriptions/index.njk-7-7 (1)
7-7: UsegovukTablemacro instead of hand-built HTML tables.The template imports the
govukTablemacro (line 2) but the subscription tables (lines 56–84, 86–112) are built with custom HTML. Per coding guidelines, Nunjucks templates must use GOV.UK Frontend component macros. Refactor both tables to usegovukTable.The concern about
casesandlocationsbeing undefined is not applicable here—the handler always provides both as arrays before rendering (see lines 27–36, 67–74).libs/notifications/prisma/schema.prisma-22-22 (1)
22-22: UseRestrictorSetNullfor audit-log retention.Line 22 cascades deletes from subscriptions to NotificationAuditLog, which permanently erases historical notification records. This contradicts audit-table design intent. With PostgreSQL's
relationMode = "foreignKeys"(default),onDelete: Cascadeenforces a realON DELETE CASCADEconstraint, deleting all audit logs when a subscription is deleted. Audit and history tables should resist or nullify such cascades to preserve compliance records. ConsideronDelete: Restrictto prevent accidental erasure, or make subscriptionId nullable and useonDelete: SetNull.libs/system-admin-pages/src/pages/list-search-config/index.njk-1-1 (1)
1-1: Use the required base layout name.This template extends
layouts/base-template.njk, but the required base islayouts/base-templates.njk. Please align to avoid layout resolution issues.Proposed fix
-{% extends "layouts/base-template.njk" %} +{% extends "layouts/base-templates.njk" %}As per coding guidelines.
libs/verified-pages/src/pages/case-name-search-results/index.njk-1-1 (1)
1-1: Use the required base layout name.This template extends
layouts/base-template.njk, but the required base islayouts/base-templates.njk.Proposed fix
-{% extends "layouts/base-template.njk" %} +{% extends "layouts/base-templates.njk" %}As per coding guidelines.
apps/postgres/prisma/migrations/20260119151000_refactor_subscription_and_add_artefact_search/migration.sql-11-14 (1)
11-14: Avoid adding NOT NULL columns without a backfill step.Adding
search_typeandsearch_valueas NOT NULL will fail ifsubscriptionalready has rows. Consider adding them nullable, backfilling, then enforcing NOT NULL in a follow-up migration.One possible migration pattern
-ALTER TABLE "subscription" -ADD COLUMN "search_type" VARCHAR(50) NOT NULL, -ADD COLUMN "search_value" TEXT NOT NULL, -DROP COLUMN IF EXISTS "location_id"; +ALTER TABLE "subscription" +ADD COLUMN "search_type" VARCHAR(50), +ADD COLUMN "search_value" TEXT, +DROP COLUMN IF EXISTS "location_id"; + +-- backfill search_type/search_value here + +ALTER TABLE "subscription" +ALTER COLUMN "search_type" SET NOT NULL, +ALTER COLUMN "search_value" SET NOT NULL;apps/postgres/scripts/migrate-subscriptions.ts-17-47 (1)
17-47: Handle missinglocationIdto avoid incomplete migration.Subscriptions with
searchType: nullbut nolocationIdare silently skipped, leavingsearchTypeunset. That can break later constraints and your verification won’t flag it. Please count/log those records and verify remaining nulls.🔧 Proposed fix
for (const subscription of subscriptions) { try { - if (subscription.locationId) { + if (subscription.locationId != null) { await prisma.subscription.update({ where: { subscriptionId: subscription.subscriptionId }, data: { searchType: "LOCATION_ID", searchValue: subscription.locationId.toString() } }); migratedCount++; + } else { + console.warn(`Skipping subscription ${subscription.subscriptionId}: missing locationId`); + errorCount++; } } catch (error) { console.error(`Failed to migrate subscription ${subscription.subscriptionId}:`, error); errorCount++; } } @@ const verifyCount = await prisma.subscription.count({ where: { searchType: "LOCATION_ID" } }); console.log(`Verification: ${verifyCount} subscriptions now have searchType='LOCATION_ID'`); + const remainingNull = await prisma.subscription.count({ where: { searchType: null } }); + if (remainingNull > 0) { + console.warn(`Verification: ${remainingNull} subscriptions still missing searchType`); + } }libs/verified-pages/src/pages/case-name-search/index.ts-59-96 (1)
59-96: Avoid logging raw case names (potential sensitive data).
caseNameis user‑provided and may be sensitive; please remove or mask it in logs.✅ Suggested change
- console.log(`[case-name-search] Searching for case name: "${caseName}"`); + console.log("[case-name-search] Searching for case name"); const results = await searchByCaseName(caseName); console.log(`[case-name-search] Found ${results.length} results`); @@ - console.error(`[case-name-search] Error searching for case name "${caseName}":`, error); + console.error("[case-name-search] Error searching for case name:", error);As per coding guidelines, avoid logging sensitive data.
libs/verified-pages/src/pages/subscription-management/index.ts-16-17 (1)
16-17: Validateviewquery parameter before use.
req.query.viewis user‑controlled; please whitelist allowed values and default safely.✅ Suggested change
- const view = (req.query.view as string) || "all"; + const viewParam = typeof req.query.view === "string" ? req.query.view : undefined; + const allowedViews = new Set(["all", "courts", "cases"]); // adjust to UI values + const view = viewParam && allowedViews.has(viewParam) ? viewParam : "all";As per coding guidelines, all endpoints must validate inputs.
libs/notifications/src/notification/notification-service.ts-68-77 (1)
68-77: User identifiers logged.Logging
userIdandsearchValue(which may contain case-identifying information) could be problematic per the guideline to not include sensitive data in logs.Suggested improvement
console.log("[notification-service] Case subscriptions found:", { totalCaseNumberSubscriptions: caseNumberSubscriptions.length, totalCaseNameSubscriptions: caseNameSubscriptions.length, - totalCaseSubscriptions: caseSubscriptions.length, - caseSubscriptionSamples: caseSubscriptions.slice(0, 3).map((cs) => ({ - userId: cs.userId, - searchValue: cs.searchValue, - caseName: cs.caseName - })) + totalCaseSubscriptions: caseSubscriptions.length });libs/verified-pages/src/pages/pending-subscriptions/index.ts-12-12 (1)
12-12: Full session data logged - potential PII exposure.Logging the entire
emailSubscriptionssession object may expose sensitive user data. Consider removing or limiting to non-sensitive fields.Suggested fix
- console.log("[pending-subscriptions] Session data:", JSON.stringify(req.session.emailSubscriptions, null, 2)); + console.log("[pending-subscriptions] Session data: pendingSubscriptions=%d, pendingCaseSubscriptions=%d", + req.session.emailSubscriptions?.pendingSubscriptions?.length || 0, + req.session.emailSubscriptions?.pendingCaseSubscriptions?.length || 0 + );libs/notifications/src/notification/notification-service.ts-45-51 (1)
45-51: Potential PII in logs.Case names and numbers could identify individuals involved in legal proceedings. Per coding guidelines, sensitive data should not be included in logs. Consider logging only counts or anonymised identifiers.
Suggested improvement
console.log("[notification-service] Artefact cases found:", { publicationId: event.publicationId, - totalCases: artefactCases.length, - caseNumbers, - caseNames, - caseSamples: artefactCases.slice(0, 3).map((ac) => ({ caseNumber: ac.caseNumber, caseName: ac.caseName })) + totalCases: artefactCases.length });libs/subscription/src/repository/queries.ts-113-122 (1)
113-122: Duplicate function:findByUserIdis identical tofindSubscriptionsByUserId.
findByUserId(lines 113-122) has the same implementation asfindSubscriptionsByUserId(lines 3-12). Consider removing the duplicate and using a single function.e2e-tests/tests/manage-list-types.spec.ts-22-90 (1)
22-90: Single journey should include validation, Welsh, accessibility, and keyboard checks.
These are currently split across separate tests; please add a consolidated journey test (or extend the full-flow test) to cover all four in one run. Based on learnings, ...libs/publication/src/artefact-search-extractor.ts-148-161 (1)
148-161: Avoid logging case identifiers and names.Case numbers and names are likely sensitive; logging samples breaches the “no sensitive data in logs” guideline. Log counts only or redact.
Suggested fix
- console.log(`[ArtefactSearch] Extracted ${cases.length} cases for artefact ${artefactId}:`, { - caseSamples: cases.slice(0, 5).map((c) => ({ caseNumber: c.caseNumber, caseName: c.caseName })) - }); + console.log(`[ArtefactSearch] Extracted ${cases.length} cases for artefact ${artefactId}`);libs/subscription/src/repository/service.ts-238-240 (1)
238-240:getSubscriptionsByCaseomits CASE_NAME subscriptions.Line 238 only fetches CASE_NUMBER, so case‑name subscriptions are excluded. Either include CASE_NAME or rename the method.
Suggested fix
export async function getSubscriptionsByCase(userId: string) { - return findByUserIdAndType(userId, "CASE_NUMBER"); + const [caseNumberSubs, caseNameSubs] = await Promise.all([ + findByUserIdAndType(userId, "CASE_NUMBER"), + findByUserIdAndType(userId, "CASE_NAME") + ]); + return [...caseNumberSubs, ...caseNameSubs]; }
🟡 Minor comments (21)
libs/verified-pages/src/pages/subscription-management/index.njk-79-79 (1)
79-79: Removearia-sort="none"if sorting is not implemented.The
aria-sort="none"attribute on the Location header suggests sortable columns, but no sorting functionality is present. This may confuse screen reader users expecting interactive sorting.🛠️ Proposed fix
- <th scope="col" class="govuk-table__header" style="width: 60%;" aria-sort="none">{{ tableHeaderLocation }}</th> + <th scope="col" class="govuk-table__header" style="width: 60%;">{{ tableHeaderLocation }}</th>Also applies to: 145-145
docs/tickets/VIBE-300/e2e-tests-summary.md-10-12 (1)
10-12: Use a relative path for portability.The absolute path contains a local machine path (
/Users/kian.kwa/...) which won't be valid for other contributors.Suggested fix
## Test File Location -`/Users/kian.kwa/IdeaProjects/cath-service/e2e-tests/tests/case-subscriptions.spec.ts` +`e2e-tests/tests/case-subscriptions.spec.ts`libs/verified-pages/src/pages/case-number-search/index.test.ts-59-71 (1)
59-71: Welsh translation test values don't matchcy.ts.Line 68 expects
cy.titleto be"Yn ôl rhif cyfeirnod yr achos, ID yr achos neu rif cyfeirnod unigryw (URN)", but the providedcy.tsdefines it as"Beth yw'r rhif cyfeirnod?".libs/verified-pages/src/pages/case-number-search/index.test.ts-43-56 (1)
43-56: Test assertions don't match actual translation values.The test at line 47 expects
en.referenceNumberHint, which is not defined inen.ts. The test at line 55 expects thetitleto be"By case reference number, case ID or unique reference number (URN)", buten.tsdefines it as"What is the reference number?". Update either the translation file or the test assertions to align them.libs/verified-pages/src/pages/case-number-search/index.test.ts-110-130 (1)
110-130: UseerrorRequiredinstead oferrorNoResultsfor empty field validation.The implementation (lines 26–46 in
index.ts) and this test both useerrorNoResultswhen the reference number is empty. This is semantically incorrect. An empty required field should useerrorRequired("Enter a reference number"), reservingerrorNoResultsfor when a search completes but returns zero results (lines 48–66). Update the handler to uset.errorRequiredandt.errorNoResultsFieldfor empty input validation.libs/list-search-config/src/config.test.ts-27-31 (1)
27-31: Cross-platform path handling issue.Using
import.meta.url.replace("file://", "")doesn't handle Windows paths correctly. On Windows,file:///C:/pathbecomes/C:/pathwhich is invalid.Suggested fix using fileURLToPath
import path from "node:path"; +import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; import * as config from "./config.js";it("should exist as a resolvable path", () => { // The path should be constructible from __dirname and ../prisma - const expectedPath = path.join(path.dirname(import.meta.url.replace("file://", "")), "../prisma"); + const expectedPath = path.join(path.dirname(fileURLToPath(import.meta.url)), "../prisma"); expect(path.normalize(config.prismaSchemas)).toBe(path.normalize(expectedPath)); });libs/verified-pages/src/pages/subscription-add/index.njk-24-41 (1)
24-41: Add fieldset with legend for accessibility compliance.GOV.UK radios should include a
fieldsetwithlegendto properly associate the question with the radio group for screen readers. TheradioLabeltranslation key exists but isn't being used.Proposed fix
{{ govukRadios({ name: "subscriptionMethod", + fieldset: { + legend: { + text: radioLabel, + classes: "govuk-fieldset__legend--m" + } + }, errorMessage: fieldErrors.subscriptionMethod if fieldErrors and fieldErrors.subscriptionMethod, items: [libs/verified-pages/src/pages/bulk-unsubscribe/index.njk-80-83 (1)
80-83: Provide a fallback for empty aria-labels.If both
caseNameandcaseNumberare missing, the label becomes empty. Please includenotAvailableas a final fallback.Proposed fix
-aria-label="Select {{ subscription.caseName or subscription.caseNumber }}" +aria-label="Select {{ subscription.caseName or subscription.caseNumber or notAvailable }}" ... -<span class="govuk-visually-hidden">Select {{ subscription.caseName or subscription.caseNumber }}</span> +<span class="govuk-visually-hidden">Select {{ subscription.caseName or subscription.caseNumber or notAvailable }}</span>Also applies to: 178-182
docs/tickets/VIBE-316/tasks.md-114-116 (1)
114-116: Minor grammar tweak for readability.Consider inserting a comma to separate clauses:
Suggested edit
-- [ ] Test publication is processed and case data extracted +- [ ] Test publication is processed, and case data extractedlibs/list-search-config/src/repository/service.test.ts-118-130 (1)
118-130: Test does not verify trimming behaviour as claimed.The test description states "should trim whitespace from valid field names before saving", but the input values
"caseNumber"and"caseName"contain no whitespace. However, the suggested fix to pass" caseNumber "would not work with the current implementation.The issue is a design mismatch:
validateFieldName()is called before trimming (line 44 of service.ts), and the pattern/^[a-zA-Z0-9_]+$/rejects strings with leading/trailing spaces. Strings with spaces would fail validation before trimming ever occurs (lines 58-60).Either rename the test to describe what it actually tests (e.g., "should save valid field names"), or refactor the implementation to trim before validation if leading/trailing spaces should be handled gracefully.
docs/tickets/VIBE-316/plan.md-175-178 (1)
175-178: Update the migration script path to match the repo.The plan references
libs/postgres/scripts/migrate-subscriptions.js, but the script lives underapps/postgres/scripts. This will mislead anyone following the runbook.📝 Proposed doc fix
- node libs/postgres/scripts/migrate-subscriptions.js + node apps/postgres/scripts/migrate-subscriptions.jslibs/subscription/src/case-search-service.ts-21-21 (1)
21-21: Edge case: records with bothcaseNumberandcaseNameas null will collide.When both fields are null, the key becomes
"|". Multiple distinct records (differentid/artefactId) would incorrectly deduplicate to one. Consider includingartefactIdin the key or handling this scenario explicitly if it's a valid data state.libs/verified-pages/src/pages/case-number-search/index.ts-27-46 (1)
27-46: Incorrect error message for empty input.Lines 35 and 38 use
t.errorNoResults("There is nothing matching your criteria") for empty input validation, butt.errorRequired("Enter a reference number") is the appropriate message. The locale files define both messages for distinct purposes.Suggested fix
return res.render("case-number-search/index", { ...t, - errors: [{ text: t.errorNoResults, href: "#referenceNumber" }], + errors: [{ text: t.errorRequired, href: "#referenceNumber" }], errorSummary: { titleText: t.errorSummaryTitle, - errorList: [{ text: t.errorNoResults, href: "#referenceNumber" }] + errorList: [{ text: t.errorRequired, href: "#referenceNumber" }] }, fieldErrors: { - referenceNumber: { text: t.errorNoResultsField } + referenceNumber: { text: t.errorRequired } },libs/verified-pages/src/pages/pending-subscriptions/index.ts-159-167 (1)
159-167: Potential nullsearchValuepassed tocreateCaseSubscription.If
searchTypeis"CASE_NAME"butcaseItem.caseNameis null,searchValuewill be null. The service may not handle this gracefully. Consider adding validation.Suggested fix
pendingCases.map((caseItem: any) => { const searchType = caseItem.searchType || "CASE_NUMBER"; const searchValue = searchType === "CASE_NAME" ? caseItem.caseName : caseItem.caseNumber; + if (!searchValue) { + console.warn("[pending-subscriptions] Skipping case with null searchValue:", caseItem.id); + return Promise.resolve(); + } return createCaseSubscription(userId, searchType, searchValue, caseItem.caseNumber, caseItem.caseName); })libs/verified-pages/src/pages/case-number-search/index.ts-87-96 (1)
87-96: Hardcoded English error message breaks Welsh support.The error messages at lines 89 and 92 are hardcoded in English. Add translations to the locale files and use them here for proper bilingual support.
Suggested approach
Add to
en.tsandcy.ts:// en.ts errorGeneric: "An error occurred while searching" // cy.ts errorGeneric: "Bu gwall wrth chwilio"Then use:
- errors: [{ text: "An error occurred while searching", href: "#referenceNumber" }], + errors: [{ text: t.errorGeneric, href: "#referenceNumber" }],libs/publication/src/repository/queries.test.ts-1202-1241 (1)
1202-1241: MissingbeforeEachblock for mock cleanup.All other test suites in this file include
beforeEach(() => { vi.clearAllMocks(); }). Adding it here would ensure consistency and prevent potential test pollution.Suggested fix
describe("deleteArtefactSearchByArtefactId", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + it("should delete all artefact search entries for given artefact ID", async () => {e2e-tests/tests/manage-list-types.spec.ts-93-109 (1)
93-109: Test name says alphabetical order, but the order isn’t asserted.
Either add an order check or rename the test to match the assertions.docs/tickets/VIBE-300/plan.md-97-101 (1)
97-101: Wording nit: avoid colon after “with”.Small grammar tweak in the bullet list.
Suggested fix
- - Return array of matching cases with: case_number, case_name, party_name (if available) + - Return array of matching cases with case_number, case_name, party_name (if available)docs/tickets/VIBE-300/plan.md-280-283 (1)
280-283: Use “log in” as a verb.Minor grammar tweak in the access‑control section.
Suggested fix
- - Redirect to login if not authenticated + - Redirect to log in if not authenticatedlibs/publication/src/artefact-search-extractor.ts-84-99 (1)
84-99: Guard against non‑object payloads beforeinchecks.
extractFromRootLevelwill throw ifjsonPayloadis a primitive. Add a type guard before usingin.Suggested fix
- if (Array.isArray(jsonPayload)) { + if (!jsonPayload || typeof jsonPayload !== "object" || Array.isArray(jsonPayload)) { return null; }docs/tickets/VIBE-300/plan.md-398-400 (1)
398-400: Hyphenate “multi‑page”.Minor spelling/style fix.
Suggested fix
-This ticket involves creating a new multi-page user journey, case search functionality, subscription management UI, and integration with existing fulfilment logic. The complexity is high due to the number of pages, search logic, and dependency on VIBE-316. +This ticket involves creating a new multi‑page user journey, case search functionality, subscription management UI, and integration with existing fulfilment logic. The complexity is high due to the number of pages, search logic, and dependency on VIBE‑316.
🧹 Nitpick comments (23)
libs/verified-pages/src/pages/subscription-management/index.njk (2)
45-72: Consider extracting case table to a reusable macro.The case table markup is duplicated between the 'all' view (lines 45-72) and 'case' view (lines 107-134). Similarly, the location table is duplicated (lines 76-101 and 142-167).
Extract these into Nunjucks macros to reduce duplication and ease future maintenance.
♻️ Suggested approach
Create macros at the top of the file or in a shared partial:
{% macro caseTable(subscriptions, tableHeaderCaseName, tableHeaderCaseNumber, tableHeaderDate, tableHeaderActions, notAvailable, removeLink, csrfToken) %} <table class="govuk-table" id="cases-table"> <thead class="govuk-table__head"> <tr class="govuk-table__row"> <th scope="col" class="govuk-table__header" style="width: 30%;">{{ tableHeaderCaseName }}</th> <th scope="col" class="govuk-table__header" style="width: 30%;">{{ tableHeaderCaseNumber }}</th> <th scope="col" class="govuk-table__header" style="width: 30%;">{{ tableHeaderDate }}</th> <th scope="col" class="govuk-table__header govuk-table__header--numeric" style="width: 10%; text-align: right;">{{ tableHeaderActions }}</th> </tr> </thead> <tbody class="govuk-table__body"> {% for subscription in subscriptions %} {# ... row content ... #} {% endfor %} </tbody> </table> {% endmacro %}Then call
{{ caseTable(caseSubscriptions, ...) }}in both views.Also applies to: 107-134
48-51: Consider moving inline styles to CSS classes.Multiple inline styles for widths, alignment, and vertical positioning are repeated throughout. Extracting these to CSS classes would improve maintainability and allow easier theming.
Also applies to: 57-60
libs/verified-pages/src/pages/case-number-search/index.test.ts (1)
36-39: Unusualconsole.errorreassignment placement.Reassigning
console.errorinafterEachmeans it only takes effect after the first test runs. If the intent is to suppress error logs during tests, move this tobeforeEachor usevi.spyOn.Suggested fix
beforeEach(() => { + vi.spyOn(console, "error").mockImplementation(() => {}); mockReq = { body: {}, session: {} as any, path: "/case-number-search", csrfToken: vi.fn(() => "mock-csrf-token") }; // ... }); afterEach(() => { vi.clearAllMocks(); - console.error = vi.fn(); });e2e-tests/tests/system-admin-dashboard.spec.ts (1)
24-27: Centralise the expected tile count to avoid repeated magic numbers. This reduces update churn when tiles change again.♻️ Suggested refactor
import AxeBuilder from "@axe-core/playwright"; import { expect, test } from "@playwright/test"; import { loginWithSSO } from "../utils/sso-helpers.js"; +const EXPECTED_TILE_COUNT = 9; + test.describe("System Admin Dashboard", () => { test.beforeEach(async ({ page }) => { @@ test("should display all 9 admin tiles", async ({ page }) => { const tiles = page.locator(".admin-tile"); - await expect(tiles).toHaveCount(9); + await expect(tiles).toHaveCount(EXPECTED_TILE_COUNT); }); @@ test("should display descriptions for all tiles", async ({ page }) => { const descriptions = page.locator(".admin-tile__description"); - await expect(descriptions).toHaveCount(9); + await expect(descriptions).toHaveCount(EXPECTED_TILE_COUNT); @@ test("should display tiles in 2-column grid", async ({ page }) => { const gridColumns = page.locator(".govuk-grid-column-one-half"); - await expect(gridColumns).toHaveCount(9); + await expect(gridColumns).toHaveCount(EXPECTED_TILE_COUNT); }); @@ test("should have accessible links `@nightly`", async ({ page }) => { const links = page.locator("a.admin-tile"); - await expect(links).toHaveCount(9); + await expect(links).toHaveCount(EXPECTED_TILE_COUNT); - for (let i = 0; i < 9; i++) { + for (let i = 0; i < EXPECTED_TILE_COUNT; i++) { await expect(links.nth(i)).toBeVisible(); } }); @@ // Verify all 9 tiles are accessible links - await expect(tileLinks).toHaveCount(9); + await expect(tileLinks).toHaveCount(EXPECTED_TILE_COUNT); }); @@ const tiles = page.locator(".admin-tile"); - await expect(tiles).toHaveCount(9); + await expect(tiles).toHaveCount(EXPECTED_TILE_COUNT); }); @@ const tiles = page.locator(".admin-tile"); - await expect(tiles).toHaveCount(9); + await expect(tiles).toHaveCount(EXPECTED_TILE_COUNT); }); @@ const tiles = page.locator(".admin-tile"); - await expect(tiles).toHaveCount(9); + await expect(tiles).toHaveCount(EXPECTED_TILE_COUNT); });Also applies to: 49-52, 64-67, 82-87, 131-133, 145-168
apps/postgres/prisma/migrations/20260122150011_add_case_name_and_case_number_to_subscription/migration.sql (1)
1-6: **Consider indexing strategy forcase_name.**The index oncase_numberis appropriate for exact lookups. Ifcase_namewill be queried usingLIKE '%...%'patterns, a standard btree index won't help—it will still perform a full table scan and ignore the index.The pg_trgm module supports GIN indexes that enable fast searching for similar strings and LIKE/ILIKE queries. If case name searches use wildcard patterns, consider adding a trigram index in a future migration.
libs/system-admin-pages/src/pages/manage-list-types/index.njk (1)
13-15: Consider adding screen reader context for the action column.The empty
<th>for the configure link column may be unclear for screen reader users. Adding visually hidden text improves accessibility.Suggested improvement
<th scope="col" class="govuk-table__header">{{ nameColumnHeading }}</th> - <th scope="col" class="govuk-table__header"></th> + <th scope="col" class="govuk-table__header"><span class="govuk-visually-hidden">{{ actionColumnHeading | default("Actions") }}</span></th>libs/verified-pages/src/pages/pending-subscriptions/index.njk (1)
56-112: Use the importedgovukTablemacro for both tables.The
govukTablemacro (already imported at line 2) can render the tables with form actions by passing HTML content in thehtmlproperty of cell objects. This aligns with the coding guideline to use GOV.UK Frontend component macros.libs/publication/src/repository/artefact-search-queries.test.ts (1)
49-73: Consider adding an empty results test forfindByCaseName.For consistency with
findByCaseNumber(line 106-112), adding a test case for when no results are found would improve test symmetry.♻️ Suggested test case
+ it("should return empty array when no match found", async () => { + vi.mocked(prisma.artefactSearch.findMany).mockResolvedValue([]); + + const result = await findByCaseName("NONEXISTENT"); + + expect(result).toEqual([]); + });docs/tickets/VIBE-316/specification.md (1)
46-50: Consider using a heading instead of bold text.Line 48 uses
**Page: List Search Configuration**which triggers MD036 (emphasis used instead of heading). Using### Page: List Search Configurationwould improve document structure and accessibility.📝 Suggested fix
### Admin Configuration Page -**Page: List Search Configuration** +### Page: List Search Configurationlibs/system-admin-pages/src/pages/manage-list-types/index.test.ts (1)
86-94: Hardcoded list type count may be fragile.The assertion
expect(listTypes.length).toBe(9)couples this test to the current mock data size. IfmockListTypeschanges, this test will fail without a clear reason.♻️ Consider importing mock data for dynamic count
+import { mockListTypes } from "@hmcts/publication"; + // ... it("should map all mock list types", async () => { await getHandler(req as Request, res as Response); const renderCall = (res.render as any).mock.calls[0]; const listTypes = renderCall[1].listTypes; - // We expect 9 list types from mock-list-types.ts - expect(listTypes.length).toBe(9); + expect(listTypes.length).toBe(mockListTypes.length); });libs/verified-pages/src/pages/subscription-add/index.ts (1)
17-17: Consider typing the CSRF token access.The
(req as any).csrfToken?.()pattern is repeated. If CSRF middleware is commonly used, consider extending the ExpressRequesttype to includecsrfTokento avoid theanycast.libs/subscription/src/case-search-service.ts (1)
20-30: Consider extracting the deduplication logic.The deduplication pattern is identical in both
searchByCaseNameandsearchByCaseReference. A shared helper would reduce duplication.♻️ Suggested helper
function deduplicateResults(results: Array<{ id: string; caseNumber: string | null; caseName: string | null; artefactId: string }>): CaseSearchResult[] { const seen = new Map<string, CaseSearchResult>(); for (const result of results) { const key = `${result.caseNumber || ""}|${result.caseName || ""}`; if (!seen.has(key)) { seen.set(key, { id: result.id, caseNumber: result.caseNumber, caseName: result.caseName, artefactId: result.artefactId }); } } return Array.from(seen.values()); }libs/list-search-config/src/repository/queries.test.ts (1)
151-155: Test name is slightly misleading."should update only specified fields" suggests partial updates, but the test updates both fields. Consider renaming to "should update with different field values" or similar.
libs/system-admin-pages/src/pages/list-search-config/[listTypeId].ts (2)
15-15: Consider adding error handling for the service call.If
service.getConfigForListTypethrows (e.g., database error), the request will fail with an unhandled exception. Consider wrapping in try/catch to render an appropriate error page.Suggested improvement
+ try { const existingConfig = await service.getConfigForListType(listTypeId); + } catch (error) { + console.error("Error fetching list search config:", error); + return res.status(500).send("Error loading configuration"); + }
51-55: Brittle field name mapping.The ternary at line 53 assumes
error.fieldis always "Case number field name" for case number errors. If the service returns different field names, mapping will default tocaseNameFieldNameincorrectly.Consider using a lookup map for clarity and robustness:
Suggested improvement
const fieldErrors: Record<string, { text: string }> = {}; + const fieldIdMap: Record<string, string> = { + "Case number field name": "caseNumberFieldName", + "Case name field name": "caseNameFieldName" + }; for (const error of result.errors) { - const fieldId = error.field === "Case number field name" ? "caseNumberFieldName" : "caseNameFieldName"; + const fieldId = fieldIdMap[error.field] || error.field; fieldErrors[fieldId] = { text: error.message }; }libs/verified-pages/src/pages/case-number-search-results/index.ts (1)
40-47: Excessive use ofanytype casts.Multiple
anycasts reduce type safety. Consider defining proper types for the session data structures to catch errors at compile time.Suggested approach
interface CaseSearchResult { id: string; caseNumber: string | null; caseName: string | null; artefactId: string; searchType?: string; } // Then use: const casesWithSearchType: CaseSearchResult[] = searchResults.map((c: CaseSearchResult) => ({ ...c, searchType: "CASE_NUMBER" }));This would require extending the session type declarations accordingly.
libs/verified-pages/src/pages/pending-subscriptions/index.ts (1)
53-57: Consider typing sort comparators.Using
anyin sort comparators loses type safety. Define interfaces for the location and case objects for better maintainability.libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (1)
26-26: Consider validatingcurrentViewinput.
currentViewis read directly from query/body without validation. If unexpected values could cause template issues, consider constraining to expected values ("all", "cases", "courts").Suggested approach
- const currentView = (req.query.view as string) || "all"; + const allowedViews = ["all", "cases", "courts"]; + const currentView = allowedViews.includes(req.query.view as string) ? (req.query.view as string) : "all";libs/publication/src/repository/queries.test.ts (1)
1042-1134: Consider adding an error handling test.Other test suites (e.g.,
createArtefact) include database error scenarios. Adding a similar test here would ensure consistent coverage.it("should handle database errors", async () => { vi.mocked(prisma.artefactSearch.create).mockRejectedValue(new Error("Database connection error")); await expect(createArtefactSearch("artefact-123", "CASE-456", "Smith vs Jones")).rejects.toThrow("Database connection error"); });libs/notifications/src/notification/subscription-queries.ts (1)
75-105: Consider extracting the shared select projection.The same select object is repeated across all three functions. Extracting it to a constant would reduce duplication.
Optional refactor
const SUBSCRIPTION_SELECT = { subscriptionId: true, userId: true, searchType: true, searchValue: true, caseName: true, caseNumber: true, user: { select: { email: true, firstName: true, surname: true } } } as const;libs/publication/src/artefact-search-extractor.test.ts (1)
38-41: Avoidanyin mocks to keep typings honest.
Consider typed helpers or explicit mock types instead ofas anyfor config and repository responses. As per coding guidelines, ...e2e-tests/tests/manage-list-types.spec.ts (1)
24-45: Prefer role/label/text selectors over CSS locators.
Switching to getByRole/getByLabel/getByText improves resilience and aligns with test guidelines. Based on learnings, ...libs/verified-pages/src/pages/case-name-search-results/index.ts (1)
18-79: Type the search result/session shapes to avoidany.
Introduce an interface for case search results and pending subscriptions so the sort/filter/map logic is type-safe. As per coding guidelines, ...
| export const en = { | ||
| title: "What is the reference number?", | ||
| heading: "What is the reference number?", | ||
| insetText: "Please enter either a case reference number, case ID or unique reference number (URN). You must enter an exact match.", | ||
| referenceNumberLabel: "Reference number", | ||
| continueButton: "Continue", | ||
| errorRequired: "Enter a reference number", | ||
| errorNoResults: "There is nothing matching your criteria", | ||
| errorNoResultsField: "Enter a valid case reference number", | ||
| errorSummaryTitle: "There is a problem" | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check test file for referenceNumberHint reference
echo "=== Searching for test file ==="
fd "case-number-search" libs/verified-pages/src/pages/ -type d
echo -e "\n=== Check for test files in the directory ==="
ls -la libs/verified-pages/src/pages/case-number-search/
echo -e "\n=== Search for referenceNumberHint in the entire case-number-search directory ==="
rg -n "referenceNumberHint" libs/verified-pages/src/pages/case-number-search/
echo -e "\n=== Check cy.ts for the same key ==="
cat -n libs/verified-pages/src/pages/case-number-search/cy.ts 2>/dev/null || echo "cy.ts not found"
echo -e "\n=== Check test file if it exists ==="
cat -n libs/verified-pages/src/pages/case-number-search/index.test.ts 2>/dev/null | head -60Repository: hmcts/cath-service
Length of output: 3878
🏁 Script executed:
#!/bin/bash
# Check more of the test file
echo "=== Full translations test section ==="
sed -n '41,80p' libs/verified-pages/src/pages/case-number-search/index.test.ts
echo -e "\n=== Check the template for referenceNumberHint usage ==="
cat -n libs/verified-pages/src/pages/case-number-search/index.njk
echo -e "\n=== Check the controller for hint usage ==="
cat -n libs/verified-pages/src/pages/case-number-search/index.tsRepository: hmcts/cath-service
Length of output: 7273
Fix missing translation keys and correct title values to match test expectations.
The test file expects en.referenceNumberHint (line 47) but the localisation object is missing this key. Additionally, the title values are incorrect:
en.titleshould be "By case reference number, case ID or unique reference number (URN)" not "What is the reference number?"cy.titleshould be "Yn ôl rhif cyfeirnod yr achos, ID yr achos neu rif cyfeirnod unigryw (URN)" not "Beth yw'r rhif cyfeirnod?"
The test also verifies that Welsh keys match English keys, which will fail while referenceNumberHint is missing from both files.
Required changes to en.ts
export const en = {
- title: "What is the reference number?",
+ title: "By case reference number, case ID or unique reference number (URN)",
heading: "What is the reference number?",
insetText: "Please enter either a case reference number, case ID or unique reference number (URN). You must enter an exact match.",
referenceNumberLabel: "Reference number",
+ referenceNumberHint: "",
continueButton: "Continue",
errorRequired: "Enter a reference number",
errorNoResults: "There is nothing matching your criteria",
errorNoResultsField: "Enter a valid case reference number",
errorSummaryTitle: "There is a problem"
};Required changes to cy.ts
export const cy = {
- title: "Beth yw'r rhif cyfeirnod?",
+ title: "Yn ôl rhif cyfeirnod yr achos, ID yr achos neu rif cyfeirnod unigryw (URN)",
heading: "Beth yw'r rhif cyfeirnod?",
insetText: "Rhowch naill ai rif cyfeirnod yr achos, ID yr achos neu rif cyfeirnod unigryw (URN). Rhaid i chi nodi union gyfatebiaeth.",
referenceNumberLabel: "Rhif cyfeirnod",
+ referenceNumberHint: "",
continueButton: "Parhau",
errorRequired: "Rhowch rif cyfeirnod",
errorNoResults: "Nid oes unrhyw beth yn cyfateb i'ch meini prawf",
errorNoResultsField: "Rhowch rif cyfeirnod achos dilys",
errorSummaryTitle: "Mae problem wedi codi"
};
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/verified-pages/src/pages/pending-subscriptions/index.test.ts (1)
80-84: Stabilise translation key parity checks by sorting.
Object.keysorder can shift if properties are rearranged, making this test brittle. Sorting avoids order sensitivity.Proposed change
- const enKeys = Object.keys(en); - const cyKeys = Object.keys(cy); + const enKeys = Object.keys(en).sort(); + const cyKeys = Object.keys(cy).sort();
| it("should sort locations alphabetically by name", async () => { | ||
| vi.mocked(locationService.getLocationById).mockImplementation((id: number) => ({ | ||
| locationId: id, | ||
| name: id === 456 ? "Zebra Court" : "Alpha Court", | ||
| welshName: id === 456 ? "Llys Zebra" : "Llys Alpha" | ||
| })); |
There was a problem hiding this comment.
Normalise mocked location IDs to avoid string/number mismatches.
pendingSubscriptions are strings in the session, so strict numeric checks can miss the intended branch and weaken the test. Coerce the ID in the mock.
Proposed change
- vi.mocked(locationService.getLocationById).mockImplementation((id: number) => ({
- locationId: id,
- name: id === 456 ? "Zebra Court" : "Alpha Court",
- welshName: id === 456 ? "Llys Zebra" : "Llys Alpha"
- }));
+ vi.mocked(locationService.getLocationById).mockImplementation((id: number | string) => {
+ const numericId = Number(id);
+ return {
+ locationId: numericId,
+ name: numericId === 456 ? "Zebra Court" : "Alpha Court",
+ welshName: numericId === 456 ? "Llys Zebra" : "Llys Alpha"
+ };
+ });- vi.mocked(locationService.getLocationById).mockImplementation((id: number) =>
- id === 456 ? null : { locationId: id, name: `Location ${id}`, welshName: `Lleoliad ${id}` }
- );
+ vi.mocked(locationService.getLocationById).mockImplementation((id: number | string) => {
+ const numericId = Number(id);
+ return numericId === 456
+ ? null
+ : { locationId: numericId, name: `Location ${numericId}`, welshName: `Lleoliad ${numericId}` };
+ });Also applies to: 248-250
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/verified-pages/src/pages/subscription-management/index.njk (1)
1-1: Extend the required base template.Line 1 extends
layouts/base-template.njk, but the guideline requireslayouts/base-templates.njk. Please align with the mandated base layout.🔧 Proposed fix
-{% extends "layouts/base-template.njk" %} +{% extends "layouts/base-templates.njk" %}As per coding guidelines, this template must extend
layouts/base-templates.njk.
🧹 Nitpick comments (3)
libs/verified-pages/src/pages/delete-subscription/index.ts (2)
38-45: Consider parallelising ownership checks.Sequential
awaitin a loop can be slow when multiple IDs are provided. UsingPromise.allwould fetch all subscriptions concurrently.♻️ Suggested refactor
try { - // Verify user owns all subscriptions - for (const id of idsArray) { - const subscription = await getSubscriptionById(id, userId); - if (!subscription) { - return res.redirect("/subscription-management"); - } + // Verify user owns all subscriptions + const subscriptions = await Promise.all( + idsArray.map((id) => getSubscriptionById(id, userId)) + ); + if (subscriptions.some((sub) => !sub)) { + return res.redirect("/subscription-management"); }
89-100: Same parallelisation opportunity as getHandler.The sequential ownership verification loop could use
Promise.allfor consistency with any refactor applied togetHandler.libs/verified-pages/src/pages/subscription-management/index.ts (1)
82-85: Potential undefined access onexisting.allSubscriptionIds.When retrieving from the Map,
existingcould theoretically be undefined if the key check and retrieval aren't atomic. The non-null assertion or a guard would make this safer.Proposed fix
} else { - const existing = seen.get(key); - existing.allSubscriptionIds.push(sub.subscriptionId); + const existing = seen.get(key); + if (existing) { + existing.allSubscriptionIds.push(sub.subscriptionId); + } }
| if (!unsubscribeConfirm) { | ||
| // Redirect to GET to show confirmation page | ||
| return res.redirect(`/delete-subscription?subscriptionId=${subscriptionId}`); | ||
| return res.redirect(`/delete-subscription?subscriptionId=${subscriptionIds}`); | ||
| } |
There was a problem hiding this comment.
URL-encode comma-separated IDs in redirect.
The subscriptionIds value contains commas which should be encoded when used in a query string to avoid potential parsing issues.
🔧 Proposed fix
if (!unsubscribeConfirm) {
// Redirect to GET to show confirmation page
- return res.redirect(`/delete-subscription?subscriptionId=${subscriptionIds}`);
+ return res.redirect(`/delete-subscription?subscriptionId=${encodeURIComponent(subscriptionIds)}`);
}Note: If encoding is applied here, the GET handler would need to decode the value, though req.query typically handles this automatically.
📝 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 (!unsubscribeConfirm) { | |
| // Redirect to GET to show confirmation page | |
| return res.redirect(`/delete-subscription?subscriptionId=${subscriptionId}`); | |
| return res.redirect(`/delete-subscription?subscriptionId=${subscriptionIds}`); | |
| } | |
| if (!unsubscribeConfirm) { | |
| // Redirect to GET to show confirmation page | |
| return res.redirect(`/delete-subscription?subscriptionId=${encodeURIComponent(subscriptionIds)}`); | |
| } |
| <table class="govuk-table" id="cases-table"> | ||
| <thead class="govuk-table__head"> | ||
| <tr class="govuk-table__row"> | ||
| <th scope="col" class="govuk-table__header" style="width: 30%;">{{ tableHeaderCaseName }}</th> | ||
| <th scope="col" class="govuk-table__header" style="width: 30%;">{{ tableHeaderCaseNumber }}</th> | ||
| <th scope="col" class="govuk-table__header" style="width: 30%;">{{ tableHeaderDate }}</th> | ||
| <th scope="col" class="govuk-table__header govuk-table__header--numeric" style="width: 10%; text-align: right;">{{ tableHeaderActions }}</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody class="govuk-table__body"> | ||
| {% for subscription in caseSubscriptions %} | ||
| <tr class="govuk-table__row"> | ||
| <td class="govuk-table__cell" style="vertical-align: middle;">{{ subscription.caseName or notAvailable }}</td> | ||
| <td class="govuk-table__cell" style="vertical-align: middle;">{{ subscription.caseNumber or notAvailable }}</td> | ||
| <td class="govuk-table__cell" style="white-space: nowrap; vertical-align: middle;">{{ subscription.dateAdded | date('D MMMM YYYY') }}</td> | ||
| <td class="govuk-table__cell govuk-table__cell--numeric" style="text-align: right;"> | ||
| <form method="post" action="/delete-subscription" style="display: inline; margin: 0; padding: 0;"> | ||
| <input type="hidden" name="_csrf" value="{{ csrfToken }}"> | ||
| <input type="hidden" name="subscriptionId" value="{{ subscription.allSubscriptionIds.join(',') if subscription.allSubscriptionIds else subscription.subscriptionId }}"> | ||
| <button type="submit" class="govuk-link govuk-button-as-link" aria-label="Remove subscription for {{ subscription.caseName or subscription.caseNumber }}"> | ||
| {{ removeLink }} | ||
| </button> | ||
| </form> | ||
| </td> | ||
| </tr> | ||
| {% endfor %} | ||
| </tbody> | ||
| </table> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists
fd "subscription-management/index.njk" libs/verified-pages/src/pages/ 2>/dev/null || echo "File path search completed"
# List files in the target directory
git ls-files "libs/verified-pages/src/pages/subscription-management/" 2>/dev/null | head -20Repository: hmcts/cath-service
Length of output: 376
🏁 Script executed:
#!/bin/bash
# Read the template file to check its content
cat -n libs/verified-pages/src/pages/subscription-management/index.njk 2>/dev/null || echo "File not found or not readable"Repository: hmcts/cath-service
Length of output: 11061
🌐 Web query:
GOV.UK Frontend Nunjucks govukTable macro documentation syntax
💡 Result:
govukTable (GOV.UK Frontend) Nunjucks macro syntax
{% from "govuk/components/table/macro.njk" import govukTable %}
{{ govukTable({
caption: "Dates and amounts",
captionClasses: "govuk-table__caption--m",
firstCellIsHeader: true,
head: [
{ text: "Date" },
{ text: "Amount" }
],
rows: [
[ { text: "First 6 weeks" }, { text: "£109.80 per week" } ],
[ { text: "Next 33 weeks" }, { text: "£109.80 per week" } ]
]
}) }}[1]
Options (summary)
rows(array, required): array of row arrays; each row is an array of cell objects. [1]head(array, optional): array of header cell objects. [1]caption(string),captionClasses(string),firstCellIsHeader(boolean),classes(string),attributes(object). [1]- Cell object (for
rowsandhead):textorhtml, plus optionalformat(for example"numeric"),classes,colspan,rowspan,attributes. [1]
Source: GOV.UK Design System “Table” component page. [1]
Use GOV.UK Frontend table macro instead of raw table markup.
The template contains four tables using hand-rolled HTML. The coding guideline requires GOV.UK Frontend component macros; replace the raw <table> blocks with the govukTable macro (imported from govuk/components/table/macro.njk). The macro supports HTML content in cells, enabling conversion of the embedded form actions.
♻️ Suggested direction (illustrative)
-{% from "govuk/components/button/macro.njk" import govukButton %}
+{% from "govuk/components/button/macro.njk" import govukButton %}
+{% from "govuk/components/table/macro.njk" import govukTable %}
-<table class="govuk-table" id="cases-table">
- ...
-</table>
+{{ govukTable({
+ attributes: { id: "cases-table" },
+ head: [
+ { text: tableHeaderCaseName },
+ { text: tableHeaderCaseNumber },
+ { text: tableHeaderDate },
+ { text: tableHeaderActions, classes: "govuk-table__header--numeric" }
+ ],
+ rows: caseSubscriptions | map(subscription => [
+ { text: subscription.caseName or notAvailable },
+ { text: subscription.caseNumber or notAvailable },
+ { text: subscription.dateAdded | date('D MMMM YYYY') },
+ { html: /* action form */ }
+ ])
+}) }}| <button type="submit" class="govuk-link govuk-button-as-link" aria-label="Remove subscription for {{ subscription.caseName or subscription.caseNumber }}"> | ||
| {{ removeLink }} |
There was a problem hiding this comment.
Provide a non-empty aria-label fallback.
If both caseName and caseNumber are missing, the aria-label becomes empty. Add notAvailable as a fallback to keep assistive text meaningful.
🔧 Proposed fix
-aria-label="Remove subscription for {{ subscription.caseName or subscription.caseNumber }}"
+aria-label="Remove subscription for {{ subscription.caseName or subscription.caseNumber or notAvailable }}"| const mockCaseSubscriptions = [ | ||
| { | ||
| subscriptionId: "sub1", | ||
| type: "case" as const, | ||
| caseName: "Test Case", | ||
| partyName: "John Doe", | ||
| referenceNumber: "REF123", | ||
| dateAdded: new Date() | ||
| } | ||
| ]; |
There was a problem hiding this comment.
Mock data structure doesn't match service return type.
The mock includes partyName and referenceNumber, but per getCaseSubscriptionsByUserId in libs/subscription/src/repository/service.ts, the service returns caseName, caseNumber, and dateAdded. This inconsistency could mask bugs if the view relies on caseNumber.
Proposed fix
const mockCaseSubscriptions = [
{
subscriptionId: "sub1",
type: "case" as const,
caseName: "Test Case",
- partyName: "John Doe",
- referenceNumber: "REF123",
+ caseNumber: "CASE123",
dateAdded: new Date()
}
];| it("should render page with both court and case subscriptions", async () => { | ||
| const mockCourtSubscriptions = [ | ||
| { | ||
| subscriptionId: "sub1", | ||
| type: "court" as const, | ||
| courtOrTribunalName: "Birmingham Crown Court", | ||
| locationId: 456, | ||
| dateAdded: new Date() | ||
| } | ||
| ]; | ||
| const mockCaseSubscriptions = [ | ||
| { | ||
| subscriptionId: "sub2", | ||
| type: "case" as const, | ||
| caseName: "Test Case", | ||
| partyName: "John Doe", | ||
| referenceNumber: "REF123", | ||
| dateAdded: new Date() | ||
| } | ||
| ]; | ||
|
|
||
| vi.mocked(subscriptionService.getAllSubscriptionsByUserId).mockResolvedValue(mockCourtSubscriptions); | ||
| vi.mocked(subscriptionService.getCaseSubscriptionsByUserId).mockResolvedValue(mockCaseSubscriptions); | ||
|
|
||
| await GET[GET.length - 1](mockReq as Request, mockRes as Response, vi.fn()); | ||
|
|
||
| expect(mockRes.render).toHaveBeenCalledWith( | ||
| "subscription-management/index", | ||
| expect.objectContaining({ | ||
| courtCount: 1, | ||
| caseCount: 1, | ||
| totalCount: 2 | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Mock data structure inconsistency.
The mock at lines 180-189 also uses partyName and referenceNumber instead of caseNumber, which doesn't align with the actual service response.
Proposed fix
const mockCaseSubscriptions = [
{
subscriptionId: "sub2",
type: "case" as const,
caseName: "Test Case",
- partyName: "John Doe",
- referenceNumber: "REF123",
+ caseNumber: "CASE123",
dateAdded: new Date()
}
];| it("should sort case subscriptions alphabetically by case name", async () => { | ||
| const mockCaseSubscriptions = [ | ||
| { | ||
| subscriptionId: "sub1", | ||
| type: "case" as const, | ||
| caseName: "Zebra Case", | ||
| partyName: "Party A", | ||
| referenceNumber: "REF1", | ||
| dateAdded: new Date() | ||
| }, | ||
| { | ||
| subscriptionId: "sub2", | ||
| type: "case" as const, | ||
| caseName: "Alpha Case", | ||
| partyName: "Party B", | ||
| referenceNumber: "REF2", | ||
| dateAdded: new Date() | ||
| } | ||
| ]; |
There was a problem hiding this comment.
Mock data inconsistent with service return type.
Same issue as above — these mocks use partyName and referenceNumber instead of caseNumber.
Proposed fix
const mockCaseSubscriptions = [
{
subscriptionId: "sub1",
type: "case" as const,
caseName: "Zebra Case",
- partyName: "Party A",
- referenceNumber: "REF1",
+ caseNumber: "CASE001",
dateAdded: new Date()
},
{
subscriptionId: "sub2",
type: "case" as const,
caseName: "Alpha Case",
- partyName: "Party B",
- referenceNumber: "REF2",
+ caseNumber: "CASE002",
dateAdded: new Date()
}
];| const sortCourtSubscriptions = (subscriptions: any[]): any[] => { | ||
| return subscriptions | ||
| .map((sub) => ({ | ||
| ...sub, | ||
| locationName: sub.courtOrTribunalName | ||
| })) | ||
| .sort((a, b) => (a.locationName || "").localeCompare(b.locationName || "")); | ||
| }; | ||
|
|
||
| const deduplicateCaseSubscriptions = (subscriptions: any[]): any[] => { | ||
| const seen = new Map<string, any>(); | ||
|
|
||
| for (const sub of subscriptions) { | ||
| const caseNumber = sub.caseNumber || ""; | ||
| const caseName = sub.caseName || ""; | ||
| const key = `${caseNumber}:${caseName}`; | ||
|
|
||
| if (!seen.has(key)) { | ||
| seen.set(key, { ...sub, allSubscriptionIds: [sub.subscriptionId] }); | ||
| } else { | ||
| const existing = seen.get(key); | ||
| existing.allSubscriptionIds.push(sub.subscriptionId); | ||
| } | ||
| } | ||
|
|
||
| return Array.from(seen.values()); | ||
| }; | ||
|
|
||
| const sortCaseSubscriptions = (subscriptions: any[]): any[] => { | ||
| return [...subscriptions].sort((a, b) => (a.caseName || "").localeCompare(b.caseName || "")); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid untyped any in helper functions.
These helper functions use any[] throughout. Per coding guidelines, strict TypeScript mode should be enabled with no any without justification. Consider defining interfaces for the subscription types.
Proposed fix
+interface CourtSubscription {
+ subscriptionId: string;
+ courtOrTribunalName?: string;
+ locationName?: string;
+ [key: string]: unknown;
+}
+
+interface CaseSubscription {
+ subscriptionId: string;
+ caseName?: string;
+ caseNumber?: string;
+ allSubscriptionIds?: string[];
+ [key: string]: unknown;
+}
+
-const sortCourtSubscriptions = (subscriptions: any[]): any[] => {
+const sortCourtSubscriptions = (subscriptions: CourtSubscription[]): CourtSubscription[] => {
return subscriptions
.map((sub) => ({
...sub,
locationName: sub.courtOrTribunalName
}))
.sort((a, b) => (a.locationName || "").localeCompare(b.locationName || ""));
};
-const deduplicateCaseSubscriptions = (subscriptions: any[]): any[] => {
- const seen = new Map<string, any>();
+const deduplicateCaseSubscriptions = (subscriptions: CaseSubscription[]): CaseSubscription[] => {
+ const seen = new Map<string, CaseSubscription>();
for (const sub of subscriptions) {
const caseNumber = sub.caseNumber || "";
const caseName = sub.caseName || "";
const key = `${caseNumber}:${caseName}`;
if (!seen.has(key)) {
seen.set(key, { ...sub, allSubscriptionIds: [sub.subscriptionId] });
} else {
- const existing = seen.get(key);
+ const existing = seen.get(key)!;
existing.allSubscriptionIds.push(sub.subscriptionId);
}
}
return Array.from(seen.values());
};
-const sortCaseSubscriptions = (subscriptions: any[]): any[] => {
+const sortCaseSubscriptions = (subscriptions: CaseSubscription[]): CaseSubscription[] => {
return [...subscriptions].sort((a, b) => (a.caseName || "").localeCompare(b.caseName || ""));
};| const subscriptionIds = req.session.emailSubscriptions?.subscriptionToRemove; | ||
|
|
||
| if (!subscriptionId) { | ||
| if (!subscriptionIds) { | ||
| return res.redirect("/subscription-management"); | ||
| } | ||
|
|
||
| // Split comma-separated IDs if present | ||
| const idsArray = subscriptionIds.split(",").map((id: string) => id.trim()); | ||
|
|
||
| try { | ||
| await removeSubscription(subscriptionId, userId); | ||
| // Delete all subscriptions | ||
| for (const id of idsArray) { | ||
| await removeSubscription(id, userId); |
There was a problem hiding this comment.
Validate and normalise subscriptionToRemove before deletion.
The endpoint only checks presence; empty/whitespace IDs or duplicates can trigger spurious errors and redirects. Ensure the value is a non‑empty string, filter blanks, and de‑duplicate before deletion.
💡 Proposed fix
- const subscriptionIds = req.session.emailSubscriptions?.subscriptionToRemove;
+ const subscriptionIds = req.session.emailSubscriptions?.subscriptionToRemove;
- if (!subscriptionIds) {
+ if (typeof subscriptionIds !== "string" || subscriptionIds.trim() === "") {
return res.redirect("/subscription-management");
}
// Split comma-separated IDs if present
- const idsArray = subscriptionIds.split(",").map((id: string) => id.trim());
+ const idsArray = Array.from(
+ new Set(subscriptionIds.split(",").map((id: string) => id.trim()).filter(Boolean))
+ );
+ if (idsArray.length === 0) {
+ return res.redirect("/subscription-management");
+ }As per coding guidelines, input validation is required on all endpoints.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
libs/verified-pages/src/pages/pending-subscriptions/index.ts (2)
79-95: Validate POST body before branching onaction.
action,locationId, andcaseIdare trusted directly; invalid combinations currently fall through without a response. Add explicit validation and return 400 for bad input. As per coding guidelines, input validation must be performed on all endpoints.Proposed guard
- const { action, locationId, caseId } = req.body; + const action = typeof req.body.action === "string" ? req.body.action : ""; + const locationId = typeof req.body.locationId === "string" ? req.body.locationId : undefined; + const caseId = typeof req.body.caseId === "string" ? req.body.caseId : undefined; + + const isValidAction = + action === "confirm" || + (action === "remove" && !!locationId) || + (action === "removeCase" && !!caseId); + if (!isValidAction) { + return res.status(400).send("Bad Request"); + }
198-206: Guard against missing case name/number before creating subscriptions.
searchValuebecomesundefinedwhen bothcaseNameandcaseNumberare absent, which can lead to invalid calls tocreateCaseSubscription. Add a guard and fail fast (or skip) before invoking the service.Example safeguard
uniqueCases.map((caseItem: any) => { const searchType = caseItem.searchType || "CASE_NUMBER"; const searchValue = searchType === "CASE_NAME" ? caseItem.caseName : caseItem.caseNumber; + if (!searchValue) { + throw new Error("Invalid case subscription data"); + } return createCaseSubscription(userId, searchType, searchValue, caseItem.caseNumber, caseItem.caseName); })
🧹 Nitpick comments (2)
libs/publication/src/artefact-search-extractor.ts (1)
4-13: Place types at the bottom to match module ordering.Module ordering calls for interfaces/types after functions; consider moving
CaseDataandCaseObjectbelow the helper and exported functions to keep structure consistent. As per coding guidelines, please keep interfaces/types at the bottom.libs/verified-pages/src/pages/pending-subscriptions/index.test.ts (1)
79-83: Stabilise translation key comparison to avoid order sensitivity.
Object.keyspreserves insertion order, so the equality check can fail if keys are reordered without changing content. Consider sorting both arrays before comparison.Suggested tweak
- const enKeys = Object.keys(en); - const cyKeys = Object.keys(cy); + const enKeys = Object.keys(en).sort(); + const cyKeys = Object.keys(cy).sort(); expect(cyKeys).toEqual(enKeys);
| function extractFromRootLevel( | ||
| jsonPayload: unknown, | ||
| caseNumberFieldName: string, | ||
| caseNameFieldName: string, | ||
| hasCaseNumberField: boolean, | ||
| hasCaseNameField: boolean | ||
| ): CaseData[] | null { | ||
| if (Array.isArray(jsonPayload)) { | ||
| return null; | ||
| } | ||
|
|
||
| const rootObj = jsonPayload as Record<string, unknown>; | ||
| const hasRootFields = (hasCaseNumberField && caseNumberFieldName in rootObj) || (hasCaseNameField && caseNameFieldName in rootObj); | ||
|
|
||
| if (!hasRootFields) { | ||
| return null; | ||
| } | ||
|
|
||
| const caseData = extractCaseDataFromObject(rootObj, caseNumberFieldName, caseNameFieldName, hasCaseNumberField, hasCaseNameField); | ||
| return caseData ? [caseData] : null; | ||
| } |
There was a problem hiding this comment.
Guard against non‑object root payloads to avoid a TypeError.
Line 95 uses the in operator on rootObj; if jsonPayload is a string/number/boolean, this throws and the extraction is skipped. Add a non‑object guard before the cast.
Suggested fix
function extractFromRootLevel(
jsonPayload: unknown,
caseNumberFieldName: string,
caseNameFieldName: string,
hasCaseNumberField: boolean,
hasCaseNameField: boolean
): CaseData[] | null {
- if (Array.isArray(jsonPayload)) {
+ if (!jsonPayload || typeof jsonPayload !== "object" || Array.isArray(jsonPayload)) {
return null;
}
const rootObj = jsonPayload as Record<string, unknown>;📝 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 extractFromRootLevel( | |
| jsonPayload: unknown, | |
| caseNumberFieldName: string, | |
| caseNameFieldName: string, | |
| hasCaseNumberField: boolean, | |
| hasCaseNameField: boolean | |
| ): CaseData[] | null { | |
| if (Array.isArray(jsonPayload)) { | |
| return null; | |
| } | |
| const rootObj = jsonPayload as Record<string, unknown>; | |
| const hasRootFields = (hasCaseNumberField && caseNumberFieldName in rootObj) || (hasCaseNameField && caseNameFieldName in rootObj); | |
| if (!hasRootFields) { | |
| return null; | |
| } | |
| const caseData = extractCaseDataFromObject(rootObj, caseNumberFieldName, caseNameFieldName, hasCaseNumberField, hasCaseNameField); | |
| return caseData ? [caseData] : null; | |
| } | |
| function extractFromRootLevel( | |
| jsonPayload: unknown, | |
| caseNumberFieldName: string, | |
| caseNameFieldName: string, | |
| hasCaseNumberField: boolean, | |
| hasCaseNameField: boolean | |
| ): CaseData[] | null { | |
| if (!jsonPayload || typeof jsonPayload !== "object" || Array.isArray(jsonPayload)) { | |
| return null; | |
| } | |
| const rootObj = jsonPayload as Record<string, unknown>; | |
| const hasRootFields = (hasCaseNumberField && caseNumberFieldName in rootObj) || (hasCaseNameField && caseNameFieldName in rootObj); | |
| if (!hasRootFields) { | |
| return null; | |
| } | |
| const caseData = extractCaseDataFromObject(rootObj, caseNumberFieldName, caseNameFieldName, hasCaseNumberField, hasCaseNameField); | |
| return caseData ? [caseData] : null; | |
| } |
| export async function extractAndStoreArtefactSearch(artefactId: string, listTypeId: number, jsonPayload: unknown): Promise<void> { | ||
| try { | ||
| const config = await getConfigForListType(listTypeId); | ||
|
|
||
| if (config && jsonPayload) { | ||
| // Extract all cases from the payload (handles both objects and arrays) | ||
| const cases = extractCases(jsonPayload, config.caseNumberFieldName, config.caseNameFieldName); | ||
|
|
||
| if (cases.length > 0) { | ||
| // Delete existing entries for this artefact to ensure idempotency | ||
| await repository.deleteArtefactSearchByArtefactId(artefactId); | ||
|
|
||
| // Create new entries for all cases | ||
| for (const caseData of cases) { | ||
| await repository.createArtefactSearch(artefactId, caseData.caseNumber, caseData.caseName); | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`[ArtefactSearch] Failed to extract/store for artefact ${artefactId}:`, error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make delete+recreate atomic to avoid partial artefact‑search rows.
If any insert fails after the delete, the artefact ends up with missing rows and the error is only logged. Consider a repository helper that performs delete+bulk create in a single transaction (and optionally rethrows so ingestion can react).
Suggested direction
- // Delete existing entries for this artefact to ensure idempotency
- await repository.deleteArtefactSearchByArtefactId(artefactId);
-
- // Create new entries for all cases
- for (const caseData of cases) {
- await repository.createArtefactSearch(artefactId, caseData.caseNumber, caseData.caseName);
- }
+ // Replace entries atomically to avoid partial updates
+ await repository.replaceArtefactSearch(artefactId, cases);| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| console.error = vi.fn(); | ||
| }); |
There was a problem hiding this comment.
console.error mock placement issue.
Assigning console.error = vi.fn() in afterEach means the first test runs with the real console.error. Move this to beforeEach or use vi.spyOn for proper restoration.
Proposed fix
beforeEach(() => {
+ vi.spyOn(console, 'error').mockImplementation(() => {});
mockReq = {
body: {},
session: {} as any,
path: "/case-number-search",
csrfToken: vi.fn(() => "mock-csrf-token")
};
mockRes = {
render: vi.fn(),
redirect: vi.fn(),
locals: {}
};
});
afterEach(() => {
vi.clearAllMocks();
- console.error = vi.fn();
+ vi.restoreAllMocks();
});
…bscription' into feature/VIBE-300
…bscription' into feature/VIBE-300
…extraction-subscription # Conflicts: # libs/publication/src/index.ts # libs/publication/src/repository/queries.test.ts # package.json # yarn.lock
…factor-artefact-search-extraction-subscription # Conflicts: # e2e-tests/tests/system-admin-dashboard.spec.ts # libs/location/prisma/schema.prisma # libs/system-admin-pages/src/pages/system-admin-dashboard/cy.ts # libs/system-admin-pages/src/pages/system-admin-dashboard/en.ts # libs/system-admin-pages/src/pages/system-admin-dashboard/index.njk.test.ts
…bscription' into feature/VIBE-300 # Conflicts: # libs/publication/src/index.ts # tsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
libs/system-admin-pages/src/pages/system-admin-dashboard/index.njk.test.ts (1)
18-78: Consider adding Welsh (cy) locale coverage.The AI summary notes that
cy.tswas also updated with a new tile. This test file only validates the English locale. As per coding guidelines, "Every page must support both English and Welsh by providing en and cy content objects to the renderer, and templates should test with ?lng=cy query parameter."libs/publication/src/repository/queries.ts (1)
275-275: Redundant comment — describes "what", not "why".The re-export statement is self-explanatory. As per coding guidelines, "Do not add comments unless meaningful - explain why, not what."
Suggested removal
-// Re-export artefact search functions from artefact-search-queries export {libs/verified-pages/src/pages/pending-subscriptions/index.ts (1)
229-245: Navigation setup is duplicated in three places.
getHandler(lines 16–19),handleConfirmerror path (lines 169–172), andrenderEmptyPendingSubscriptions(lines 230–233) all repeat the same navigation-initialisation block. Consider extracting a small helper to keep it DRY.Also applies to: 15-31
| } | ||
|
|
||
| model ArtefactSearch { | ||
| id String @id @default(uuid()) |
There was a problem hiding this comment.
Missing @db.Uuid on primary key.
Every other UUID primary key in this schema (Artefact.artefactId, User.userId, MediaApplication.id, IngestionLog.id) specifies @db.Uuid to use a native PostgreSQL uuid column type. Without it, this id defaults to text, which is inconsistent and less efficient for indexing and storage.
Proposed fix
- id String `@id` `@default`(uuid())
+ id String `@id` `@default`(uuid()) `@db.Uuid`📝 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.
| id String @id @default(uuid()) | |
| id String `@id` `@default`(uuid()) `@db.Uuid` |
| if (action === "confirm") { | ||
| if (pendingLocationIds.length === 0) { | ||
| return res.redirect("/location-name-search"); | ||
| return handleConfirm(req, res, userId, pendingLocationIds, pendingCases, locale, t); | ||
| } | ||
| }; |
There was a problem hiding this comment.
postHandler silently drops requests with unrecognised actions.
If action doesn't match "remove", "removeCase", or "confirm", execution falls off the end and the response hangs indefinitely. Add a fallback.
Proposed fix
if (action === "confirm") {
return handleConfirm(req, res, userId, pendingLocationIds, pendingCases, locale, t);
}
+
+ return res.redirect("/pending-subscriptions");
};| locale: string, | ||
| t: typeof en | ||
| ) => { | ||
| req.session.emailSubscriptions.pendingSubscriptions = pendingLocationIds.filter((id: string) => id !== locationId); |
There was a problem hiding this comment.
Potential NPE when accessing req.session.emailSubscriptions without a null guard.
Lines 81–82 use optional chaining (?.), but the helper functions write directly to req.session.emailSubscriptions.pendingSubscriptions (line 106), .pendingCaseSubscriptions (line 119), and several properties in handleConfirm (lines 145–149). If emailSubscriptions is undefined, these assignments throw at runtime.
Proposed fix — initialise before mutating
+ req.session.emailSubscriptions ??= {};
req.session.emailSubscriptions.pendingSubscriptions = pendingLocationIds.filter((id: string) => id !== locationId);Apply the same guard at the top of handleRemoveCase and handleConfirm.
Also applies to: 119-119, 145-149
| res.render("pending-subscriptions/index", { | ||
| ...t, | ||
| errors: { | ||
| titleText: t.errorSummaryTitle, | ||
| errorList: [{ text: errorMessage }] | ||
| }, | ||
| locations: pendingLocations, | ||
| cases: pendingCases, | ||
| isPlural | ||
| }); |
There was a problem hiding this comment.
csrfToken missing from the error re-render in handleConfirm.
The GET handler passes csrfToken (line 66), but the error render here omits it. If the template's form includes a CSRF field, resubmission from this page will fail.
Proposed fix
res.render("pending-subscriptions/index", {
...t,
errors: {
titleText: t.errorSummaryTitle,
errorList: [{ text: errorMessage }]
},
locations: pendingLocations,
cases: pendingCases,
- isPlural
+ isPlural,
+ csrfToken: (req as any).csrfToken?.() || ""
});| "@hmcts/subscriptions": ["libs/subscription/src"], | ||
| "@hmcts/subscriptions/config": ["libs/subscription/src/config"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which form is used in imports across the codebase
echo "=== Imports using `@hmcts/subscriptions` ==="
rg -n "@hmcts/subscriptions" --type=ts --type=tsx -g '!node_modules' -g '!dist' 2>/dev/null | head -30
echo ""
echo "=== Imports using `@hmcts/subscription` (singular, no trailing s) ==="
rg -n "@hmcts/subscription[^s]" --type=ts --type=tsx -g '!node_modules' -g '!dist' 2>/dev/null | head -30
echo ""
echo "=== Verify libs/subscription directory exists ==="
fd -t d '^subscription$' libs/ --max-depth 1 2>/dev/null
echo ""
echo "=== Verify libs/subscriptions directory does NOT exist ==="
fd -t d '^subscriptions$' libs/ --max-depth 1 2>/dev/null
echo ""
echo "=== Check current tsconfig.json entries for subscriptions ==="
rg -A2 "@hmcts/subscriptions" tsconfig.json 2>/dev/null | head -20Repository: hmcts/cath-service
Length of output: 608
Alias name does not match directory: change @hmcts/subscriptions to @hmcts/subscription
The alias @hmcts/subscriptions (plural) points to libs/subscription/src (singular), which violates the coding guideline requiring @hmcts/* aliases to match libs/[module]/src naming. Update line 35 to "@hmcts/subscription" and line 36 to "@hmcts/subscription/config" to align the alias with the actual directory name.
…bscription' into feature/VIBE-300
…bscription' into feature/VIBE-300
|
2 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-300
Change description
Add subscription by case name and case number
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation