VIBE-306 - Verified user bulk unsubscribe #177
Conversation
Generated specification and implementation plan for verified user bulk unsubscribe functionality with tabbed views and confirmation workflow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughAdds a bulk-unsubscribe feature with selection, confirmation, and success pages; localization, templates, and client select-all JS; refactors subscriptions queries/service for user-scoped lookups and transactional bulk delete; updates Vite asset inputs; adds unit and E2E tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant WebServer as VerifiedPages
participant Service as SubscriptionsService
participant DB as Database
User->>Browser: open /bulk-unsubscribe
Browser->>WebServer: GET /bulk-unsubscribe
WebServer->>Service: getAllSubscriptionsByUserId(userId, locale)
Service->>DB: query subscriptions + location data
DB-->>Service: subscriptions
Service-->>WebServer: mapped DTOs
WebServer-->>Browser: render tabs & checkboxes
Browser->>WebServer: POST selected IDs
WebServer->>WebServer: validate & store req.session.bulkUnsubscribe
WebServer-->>Browser: redirect /confirm-bulk-unsubscribe
Browser->>WebServer: GET /confirm-bulk-unsubscribe
WebServer->>Service: getSubscriptionDetailsForConfirmation(ids, userId, locale)
Service->>DB: fetch details by ids
DB-->>Service: details
Service-->>WebServer: mapped details
WebServer-->>Browser: render confirmation
Browser->>WebServer: POST confirm=yes
WebServer->>Service: deleteSubscriptionsByIds(ids, userId)
Service->>DB: transactional delete by ids+userId
DB-->>Service: deletion result
Service-->>WebServer: deletion count
WebServer->>WebServer: clear session.bulkUnsubscribe
WebServer-->>Browser: redirect /bulk-unsubscribe-success
Browser->>WebServer: GET /bulk-unsubscribe-success
WebServer-->>Browser: render success page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
🧹 Nitpick comments (13)
docs/tickets/VIBE-306/plan.md (1)
25-47: Optional: Add language identifier to fenced code block.The directory structure code block would benefit from a language identifier for better syntax highlighting and compliance with markdown best practices.
Apply this change:
-``` +```text libs/bulk-unsubscribe/ ├── package.jsonAs per coding guidelines, this is a pedantic detail that can be deferred.
docs/tickets/VIBE-306/specification.md (1)
20-68: Optional: Add language identifier to fenced code block.The user journey flow diagram would benefit from a language identifier (e.g.,
textorplaintext) for better rendering and markdown best practices.Apply this change:
-``` +```text START |Note: The bare URLs flagged by markdownlint at lines 74, 119, and 155 are appropriate in this specification context as they document actual endpoint URLs.
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.ts (1)
6-24: Success handler correctly clears bulk unsubscribe state and initialises navigationThe GET handler safely:
- falls back to
"en"locale,- guards unauthenticated access,
- clears any
req.session.bulkUnsubscribeobject,- initialises
res.locals.navigationand setsverifiedItems,- and renders the success template with the translation object.
This matches the patterns used elsewhere in the verified pages flow. The explicit
req.user?.idcheck is slightly redundant withrequireAuth()in the middleware array but also keeps the handler safe when invoked directly in tests, so it’s reasonable to keep.libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.njk (1)
10-96: Template structure and error handling look sound; minor heading behaviour nitThe page:
- uses GOV.UK macros correctly (error summary, radios, button),
- correctly wires error summary and
errorMessage: errors[0]to theconfirmradios withhref: "#confirm",- conditionally renders case and court tables based on
hasCaseSubscriptions/hasCourtSubscriptions,- and posts back to
/confirm-bulk-unsubscribewith a CSRF token.One minor UX point: the court subscriptions heading (
tabSubscriptionsByCourt) is only rendered when there are also case subscriptions. If you ever have court-only subscriptions, you may want to render that heading even whenhasCaseSubscriptionsis false so the table isn’t “floating” without a section title.libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts (1)
44-200: Confirm-bulk-unsubscribe tests align well with controller logic; consider future case-subscription scenarioThese tests exercise the main GET/POST flows (session presence, error handling, confirmation yes/no branches, deletion errors, and auth redirects) and assert the key interactions with
@hmcts/subscriptions. Once the controller starts distinguishing between case and court subscriptions for confirmation, it would be worth adding a scenario wheregetSubscriptionDetailsForConfirmationreturns both types and asserting the expected split betweencaseSubscriptionsandcourtSubscriptionsin the render context.e2e-tests/tests/bulk-unsubscribe.spec.ts (4)
47-51: Type safety issue with mixed array destructuring.The destructured values from the mixed array lack explicit type annotations. TypeScript will infer
(string | number)[], makingida union type when it should benumber.Consider using explicit typing or restructuring:
- for (const [id, name, welshName] of [ - [locationId1, locationName1, locationWelshName1], - [locationId2, locationName2, locationWelshName2], - [locationId3, locationName3, locationWelshName3], - ]) { + const locationsToCreate: Array<{ id: number; name: string; welshName: string }> = [ + { id: locationId1, name: locationName1, welshName: locationWelshName1 }, + { id: locationId2, name: locationName2, welshName: locationWelshName2 }, + { id: locationId3, name: locationName3, welshName: locationWelshName3 }, + ]; + + for (const { id, name, welshName } of locationsToCreate) { await prisma.location.upsert({ - where: { locationId: id }, + where: { locationId: id },
126-130: Consider adding a guard for missing environment variables.The non-null assertions (
!) will throw a cryptic error if the environment variables are not set. A descriptive error message would improve debugging.+ const testEmail = process.env.CFT_VALID_TEST_ACCOUNT; + const testPassword = process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD; + if (!testEmail || !testPassword) { + throw new Error("CFT_VALID_TEST_ACCOUNT and CFT_VALID_TEST_ACCOUNT_PASSWORD must be set"); + } + await loginWithCftIdam( page, - process.env.CFT_VALID_TEST_ACCOUNT!, - process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD! + testEmail, + testPassword );
135-141: Minor: Empty destructuring is unconventional.The
{}in the callback signature is unused. Consider using_prefix or omitting if Playwright allows.- test.afterEach(async ({}, testInfo) => { + test.afterEach(async (_, testInfo) => {
155-155: Consider replacingnetworkidlewith more reliable wait strategies.
waitForLoadState("networkidle")can be flaky and is not recommended for modern Playwright tests. Consider using more specific waits.- await page.waitForLoadState("networkidle"); + await page.waitForLoadState("domcontentloaded");Alternatively, wait for a specific element that indicates the page is ready:
await page.locator(`#location-${testData.locationId1}`).waitFor({ state: 'visible' });libs/verified-pages/src/pages/bulk-unsubscribe/index.njk (1)
54-57: Inline styles should be moved to CSS.Multiple
style="vertical-align: middle;"attributes are used throughout. This should be extracted to a CSS class for maintainability and consistency.Consider adding a utility class:
.govuk-table__cell--valign-middle { vertical-align: middle; }Then apply it to the elements instead of inline styles.
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (3)
49-50: Redundant API calls whenviewis "all".When
view === "all",caseSubscriptionsandcourtSubscriptionsare already populated from lines 34-35, but lines 49-50 conditionally re-fetch or use the existing data. This logic causes unnecessary re-fetches whenviewis not "all".Simplify by always fetching all subscriptions for count purposes, or cache counts separately:
- const allCaseSubscriptions = view === "all" || view === "case" ? caseSubscriptions : await getCaseSubscriptionsByUserId(userId, locale); - const allCourtSubscriptions = view === "all" || view === "court" ? courtSubscriptions : await getCourtSubscriptionsByUserId(userId, locale); + // Always fetch all subscriptions once for accurate counts + const allCaseSubscriptions = view === "all" ? caseSubscriptions : await getCaseSubscriptionsByUserId(userId, locale); + const allCourtSubscriptions = view === "all" ? courtSubscriptions : await getCourtSubscriptionsByUserId(userId, locale);Or better, fetch all subscriptions once at the start and filter for display based on
view.
99-99: Simplify subscription parsing logic.The nested ternary is hard to read. Consider using a more explicit approach.
- const selectedIds = Array.isArray(req.body.subscriptions) ? req.body.subscriptions : req.body.subscriptions ? [req.body.subscriptions] : []; + const rawSubscriptions = req.body.subscriptions; + const selectedIds: string[] = Array.isArray(rawSubscriptions) + ? rawSubscriptions + : rawSubscriptions + ? [rawSubscriptions] + : [];
105-145: Significant code duplication with GET handler.Lines 105-145 duplicate most of the logic from
getHandler(lines 29-65). This violates DRY principles and increases maintenance burden.Extract the common rendering logic into a shared helper:
interface RenderOptions { view: string; errors?: Array<{ text: string; href: string }>; } async function renderBulkUnsubscribePage( req: Request, res: Response, userId: string, locale: string, options: RenderOptions ): Promise<void> { const t = locale === "cy" ? cy : en; const { view, errors } = options; let caseSubscriptions = []; let courtSubscriptions = []; if (view === "all" || view === "case") { caseSubscriptions = await getCaseSubscriptionsByUserId(userId, locale); } if (view === "all" || view === "court") { courtSubscriptions = await getCourtSubscriptionsByUserId(userId, locale); } // ... rest of shared logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/web/src/assets/css/index.scss(1 hunks)apps/web/vite.build.ts(2 hunks)docs/tickets/VIBE-306/plan.md(1 hunks)docs/tickets/VIBE-306/specification.md(1 hunks)e2e-tests/tests/bulk-unsubscribe.spec.ts(1 hunks)libs/subscriptions/src/repository/service.test.ts(2 hunks)libs/subscriptions/src/repository/service.ts(1 hunks)libs/verified-pages/src/assets/js/select_all.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe-success/cy.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe-success/en.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe-success/index.njk(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe-success/index.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/cy.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/en.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/index.njk(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/index.test.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/index.ts(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.ts(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.ts(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.njk(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts(1 hunks)libs/verified-pages/src/pages/subscription-management/cy.ts(1 hunks)libs/verified-pages/src/pages/subscription-management/en.ts(1 hunks)libs/verified-pages/src/pages/subscription-management/index.njk(1 hunks)libs/web-core/src/views/layouts/base-template.njk(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/subscription-management/en.tse2e-tests/tests/bulk-unsubscribe.spec.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/cy.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.tslibs/verified-pages/src/pages/bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.test.tslibs/verified-pages/src/assets/js/select_all.tslibs/verified-pages/src/pages/bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.tsapps/web/vite.build.ts
**/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.ts: Pages are registered through explicit imports inapps/web/src/app.ts. Routes are created based on file names within thepages/directory (e.g.,my-page.tsbecomes/my-page, nested routes via subdirectories).
Page controller files must exportGETand/orPOSTfunctions that accept Express Request and Response, render usingres.render(), and handle form submissions.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/cy.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.tslibs/verified-pages/src/pages/bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/cy.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.tslibs/verified-pages/src/pages/subscription-management/index.njklibs/verified-pages/src/pages/bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.njklibs/verified-pages/src/pages/bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.njklibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.njklibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/subscription-management/en.tse2e-tests/tests/bulk-unsubscribe.spec.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/cy.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.tslibs/verified-pages/src/pages/bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.test.tslibs/verified-pages/src/assets/js/select_all.tslibs/verified-pages/src/pages/bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.tsapps/web/vite.build.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/subscription-management/en.tse2e-tests/tests/bulk-unsubscribe.spec.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/cy.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.tslibs/verified-pages/src/pages/bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.test.tslibs/verified-pages/src/assets/js/select_all.tslibs/verified-pages/src/pages/bulk-unsubscribe/en.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.tsapps/web/vite.build.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/bulk-unsubscribe.spec.ts
**/src/pages/**/*.njk
📄 CodeRabbit inference engine (CLAUDE.md)
Nunjucks templates must extend
layouts/base-templates.njk, use govuk macros for components, include error summaries, and support conditional rendering based on language variables.
Files:
libs/verified-pages/src/pages/subscription-management/index.njklibs/verified-pages/src/pages/bulk-unsubscribe/index.njklibs/verified-pages/src/pages/bulk-unsubscribe-success/index.njklibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.njk
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts
🧠 Learnings (11)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/en.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/cy.ts
📚 Learning: 2025-11-20T09:59:16.776Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 106
File: libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts:84-160
Timestamp: 2025-11-20T09:59:16.776Z
Learning: In the cath-service repository, Welsh localization (lng=cy) is not required for admin screens (system-admin-pages), so locale preservation in admin screen redirects is not necessary.
Applied to files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.tslibs/verified-pages/src/pages/bulk-unsubscribe/cy.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/bulk-unsubscribe.spec.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/bulk-unsubscribe.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.ts : Page controller files must export `GET` and/or `POST` functions that accept Express Request and Response, render using `res.render()`, and handle form submissions.
Applied to files:
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.tslibs/verified-pages/src/pages/bulk-unsubscribe/index.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.ts : Pages are registered through explicit imports in `apps/web/src/app.ts`. Routes are created based on file names within the `pages/` directory (e.g., `my-page.ts` becomes `/my-page`, nested routes via subdirectories).
Applied to files:
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk`, use govuk macros for components, include error summaries, and support conditional rendering based on language variables.
Applied to files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.njklibs/verified-pages/src/pages/bulk-unsubscribe-success/index.njklibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.njk
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/libs/*/src/config.ts : Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate `config.ts` file to avoid circular dependencies during Prisma client generation. Apps import config using the `/config` path.
Applied to files:
apps/web/vite.build.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.{ts,tsx} : Use workspace aliases (`hmcts/*`) for imports between packages instead of relative paths.
Applied to files:
apps/web/vite.build.ts
🧬 Code graph analysis (9)
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.ts (1)
libs/verified-pages/src/pages/bulk-unsubscribe/cy.ts (1)
cy(1-29)
libs/verified-pages/src/pages/bulk-unsubscribe-success/en.ts (3)
libs/verified-pages/src/pages/bulk-unsubscribe/en.ts (1)
en(1-29)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.ts (1)
en(1-17)libs/verified-pages/src/pages/subscription-management/en.ts (1)
en(1-11)
e2e-tests/tests/bulk-unsubscribe.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
libs/verified-pages/src/pages/bulk-unsubscribe/cy.ts (3)
libs/verified-pages/src/pages/bulk-unsubscribe-success/cy.ts (1)
cy(1-11)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.ts (1)
cy(1-17)libs/verified-pages/src/pages/subscription-management/cy.ts (1)
cy(1-11)
libs/verified-pages/src/pages/bulk-unsubscribe/index.test.ts (3)
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.ts (1)
GET(26-26)libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (2)
GET(161-161)POST(162-162)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts (2)
GET(115-115)POST(116-116)
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts (3)
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.ts (1)
GET(26-26)libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (2)
GET(161-161)POST(162-162)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts (2)
GET(115-115)POST(116-116)
libs/subscriptions/src/repository/service.test.ts (1)
libs/subscriptions/src/repository/service.ts (6)
getAllSubscriptionsByUserId(117-127)getCaseSubscriptionsByUserId(129-133)getCourtSubscriptionsByUserId(135-137)validateSubscriptionOwnership(139-159)getSubscriptionDetailsForConfirmation(161-177)deleteSubscriptionsByIds(179-201)
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts (3)
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.ts (1)
GET(26-26)libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (1)
GET(161-161)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts (1)
GET(115-115)
apps/web/vite.build.ts (2)
libs/web-core/src/index.ts (1)
createBaseViteConfig(4-4)libs/web-core/src/assets/vite-config.ts (1)
createBaseViteConfig(11-70)
🪛 markdownlint-cli2 (0.18.1)
docs/tickets/VIBE-306/specification.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
74-74: Bare URL used
(MD034, no-bare-urls)
119-119: Bare URL used
(MD034, no-bare-urls)
155-155: Bare URL used
(MD034, no-bare-urls)
docs/tickets/VIBE-306/plan.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (42)
libs/verified-pages/src/pages/bulk-unsubscribe-success/cy.ts (1)
1-11: LGTM! Welsh localization properly implemented.The Welsh translations are complete and correctly mirror the English localization structure. Link paths are appropriately left untranslated and match the English version.
apps/web/vite.build.ts (2)
3-5: LGTM! Asset configuration properly extended.The imports and asset configuration correctly include the new verified-pages and admin-pages assets using workspace aliases, aligning with the coding guidelines.
Also applies to: 14-14
18-26: LGTM! Rollup input correctly extended for web-specific assets.The build configuration properly merges the base input with new web_css and web_js entry points. The use of optional chaining is appropriate here to safely access potentially undefined nested properties.
libs/web-core/src/views/layouts/base-template.njk (1)
7-7: LGTM! Template asset reference updated correctly.The stylesheet reference change from
index_csstoweb_csscorrectly aligns with the new Vite build configuration that definesweb_cssas a Rollup input entry point.apps/web/src/assets/css/index.scss (1)
10-10: LGTM! SCSS import path updated correctly.The import path change from
index.scsstoverified-pages.scssaligns with the verified-pages package restructuring while preserving the "verified" alias for consistent usage.docs/tickets/VIBE-306/plan.md (1)
1-310: LGTM! Comprehensive implementation plan.The technical plan thoroughly covers all aspects of the bulk unsubscribe feature implementation, including architecture, security considerations, testing strategy, and accessibility requirements. The plan provides clear guidance for developers implementing VIBE-306.
libs/verified-pages/src/pages/bulk-unsubscribe-success/en.ts (1)
1-11: LGTM! English localization properly implemented.The English translations are complete and correctly structured. Keys and paths are consistent with the Welsh localization, ensuring proper bilingual support.
libs/verified-pages/src/pages/bulk-unsubscribe/cy.ts (1)
1-29: LGTM! Welsh localization complete and consistent.The Welsh translations for the bulk unsubscribe page are properly implemented with all required keys matching the English version. The structure follows the established patterns for other verified-pages localizations.
docs/tickets/VIBE-306/specification.md (1)
1-210: LGTM! Comprehensive feature specification.The specification document thoroughly details all aspects of the bulk unsubscribe feature, including user journey, page content (with Welsh placeholder indicators), validation rules, accessibility requirements, and test scenarios. This provides clear requirements for VIBE-306 implementation.
libs/verified-pages/src/pages/bulk-unsubscribe/en.ts (1)
1-29: LGTM!The localization structure is well-organized with clear groupings (tabs, table headers, buttons, empty state, errors). All keys follow camelCase naming convention as required.
libs/subscriptions/src/repository/service.ts (6)
104-115: LGTM!The locale-aware mapping correctly falls back to English names when Welsh translations are unavailable. The type literal "court" aligns with the current implementation.
117-127: LGTM!The function correctly fetches subscriptions with location data, orders by newest first, and applies locale-aware mapping.
129-133: LGTM!The placeholder implementation is appropriate given that case subscriptions are not yet functional per the PR description. Underscore prefixes correctly indicate unused parameters.
135-137: LGTM!The wrapper function is appropriate for the current implementation where only court subscriptions exist.
139-159: LGTM!The ownership validation is correctly implemented with proper checks for:
- Empty input
- All subscriptions exist (count match)
- All subscriptions belong to the user
The use of
selectto fetch only required fields is a good performance optimization.
161-177: LGTM!The function correctly fetches subscription details filtered by IDs with locale support. The early return for empty input avoids unnecessary database queries.
libs/subscriptions/src/repository/service.test.ts (4)
23-27: LGTM!The mock setup correctly includes the additional Prisma methods needed for testing the new bulk operations (
findMany,deleteMany,$transaction).
309-381: LGTM!Comprehensive test coverage for locale handling including:
- English locale rendering
- Welsh locale with and without Welsh names
- Empty result scenarios
383-558: LGTM!Excellent test coverage including:
- Edge cases (empty arrays, missing subscriptions)
- Security scenarios (unauthorized access)
- Localization behavior
- Placeholder test for unimplemented feature
560-663: LGTM!Thorough test coverage for the deletion function including:
- Transactional behavior
- Error handling (empty input, unauthorized, missing subscriptions)
- Database error rollback scenarios
- Verification of correct where clause usage
- Edge case of zero deletions
libs/verified-pages/src/pages/subscription-management/en.ts (1)
6-6: LGTM!The new localization key is consistent with existing entries and has Welsh translation parity.
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/cy.ts (1)
1-17: LGTM!Welsh localization is properly structured with all required keys matching the English counterpart. The translations cover all user-facing text including error messages and form controls.
libs/verified-pages/src/pages/subscription-management/cy.ts (1)
6-6: LGTM!Welsh translation maintains localization parity with the English version.
libs/verified-pages/src/pages/bulk-unsubscribe-success/index.njk (1)
1-31: LGTM!The template correctly:
- Extends the base layout per guidelines
- Uses GOV.UK components with proper accessibility attributes (
role="status",aria-live="polite")- Supports localization through variable interpolation
- Follows the standard panel pattern for success pages
libs/verified-pages/src/assets/js/select_all.ts (1)
1-26: The select-all checkbox logic is correctly implemented with proper indeterminate state handling. No security concerns regarding selector injection are present—thetableIdvalues (case-subscriptions-table,court-subscriptions-table) are hardcoded static strings in the template, not derived from user input.libs/verified-pages/src/pages/subscription-management/index.njk (1)
14-27: Bulk unsubscribe button integration looks consistent and conditional use is correctWrapping the existing add button in a
govuk-button-groupand only showing the secondary bulk unsubscribe button whencount > 0aligns with the existing table conditional and GOV.UK styling; the href and text wiring look correct assumingbulkUnsubscribeButtonis present in bothenandcytranslations.libs/verified-pages/src/pages/bulk-unsubscribe-success/index.test.ts (1)
5-65: Good coverage of success-page behavioursThe tests exercise the key behaviours of the success handler (render, clearing
bulkUnsubscribefrom session, unauthenticated redirect, and handling missing session data) using a simple mock of@hmcts/auth. This gives solid confidence in the page wiring; any further assertions around navigation setup would be nice-to-have, not required.libs/verified-pages/src/pages/confirm-bulk-unsubscribe/en.ts (1)
1-17: English localisation object matches template and controller needsAll keys required by the confirm template and controller (
confirmTitle, table headers, radio labels, error message anderrorNoRadioHref) are present and correctly named;errorNoRadioHref: "#confirm"lines up with the radios group id, so error links will focus the right control.libs/verified-pages/src/pages/bulk-unsubscribe/index.test.ts (1)
40-284: Comprehensive bulk-unsubscribe tests give strong coverage of the flowThe GET and POST tests collectively cover the important branches: per-view filtering, empty state, prior selections in session, validation on missing selections, and auth redirects, all with
@hmcts/subscriptionsmocked in a clear way. This is a solid, maintainable test suite for the page.e2e-tests/tests/bulk-unsubscribe.spec.ts (4)
1-5: LGTM!Imports follow ESM conventions with
.jsextensions for relative imports, and workspace aliases (@hmcts/postgres) are correctly used for cross-package imports. The Playwright and Axe-core imports are properly structured. Based on coding guidelines compliance.
6-18: LGTM!Interface naming follows PascalCase convention. Using a
Mapkeyed bytestIdfor test data isolation is a good pattern for parallel test execution.
93-113: LGTM!The deletion order (subscriptions before locations) correctly handles foreign key constraints. Error handling during cleanup prevents test failures from masking actual test issues.
143-410: Comprehensive E2E test covering required aspects.This test covers:
- Complete user journey (subscription creation → bulk unsubscribe → verification)
- Accessibility checks with Axe-core (Steps 3, 14, 19)
- Welsh translation verification (Steps 7, 20)
- Keyboard navigation (Steps 5, 16)
- Database state verification (Steps 21-22)
- Error validation (Steps 8, 15)
- Empty state handling (Step 25)
As per coding guidelines for E2E tests.
libs/verified-pages/src/pages/bulk-unsubscribe/index.njk (5)
1-9: LGTM!Template correctly extends the base layout, imports required GOV.UK macros, and constructs the page title using localized variables.
58-70: LGTM!Select-all checkbox has proper accessibility attributes:
aria-labelfor screen readers and visually hidden label text. Thedata-tableattribute provides a clean hook for the JavaScript behavior.
82-92: LGTM!Row checkboxes correctly use
name="subscriptions"for form array submission and include proper accessible labels. The pre-selection logic usingpreviouslySelectedmaintains state across page loads.
100-147: LGTM!Court subscriptions table follows the same accessible patterns as the case subscriptions table. The conditional heading (only shown when both tables are present) improves visual hierarchy.
45-48: LGTM!Form includes CSRF protection and preserves the current view state via hidden field. The
novalidateattribute correctly delegates validation to the server.libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (4)
1-5: LGTM!Imports follow ESM conventions with
.jsextensions for relative imports and use workspace aliases for cross-package imports.
7-16: LGTM!Session type augmentation properly extends
express-sessionwith thebulkUnsubscribedata structure. Interface naming follows PascalCase convention.
161-162: LGTM!Handlers are correctly exported with authentication middleware (
requireAuth,blockUserAccess). This follows the page controller pattern per coding guidelines.
30-31: Replaceany[]with explicit subscription types.Using
any[]violates TypeScript strict mode requirements. Identify the return types from the subscription service functions and apply them directly. Check what types are exported from@hmcts/subscriptionsand import them to properly type these arrays, or useAwaited<ReturnType<typeof functionName>>if appropriate type exports don't exist.
🎭 Playwright E2E Test Results182 tests 182 ✅ 15m 11s ⏱️ Results for commit 6e6f588. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/subscriptions/src/repository/service.ts (1)
169-169: RemoveuserIdfrom log message to prevent PII logging.This was flagged in a previous review. The log includes user-identifiable information which should not be logged. Remove the userId from the message:
- console.log(`Bulk unsubscribe: User ${userId} deleted ${count} subscriptions`); + console.log(`Bulk unsubscribe: deleted ${count} subscriptions`);
🧹 Nitpick comments (3)
libs/verified-pages/src/pages/pending-subscriptions/index.ts (1)
102-102: Consider passing locale parameter for consistency.The
getAllSubscriptionsByUserIdfunction accepts alocaleparameter (with default"en"). For consistency with other pages (e.g.,subscription-management/index.tsat line 18), consider passing the locale explicitly:- const existingSubscriptions = await getAllSubscriptionsByUserId(userId); + const existingSubscriptions = await getAllSubscriptionsByUserId(userId, locale);This ensures consistent locale-aware data retrieval across all pages.
libs/subscriptions/src/repository/service.test.ts (1)
352-409: LGTM! Tests correctly verify court subscription retrieval.The tests properly verify locale handling. Note that
mockSubscriptionsis duplicated betweengetAllSubscriptionsByUserIdandgetCourtSubscriptionsByUserIdtest blocks—consider extracting to a shared constant at the describe block level for maintainability, but this is a minor nit.libs/verified-pages/src/assets/js/select_all.test.ts (1)
6-11: Consider removing redundant afterEach.The
afterEachhook performs the same cleanup asbeforeEach. SincebeforeEachalready clears the DOM before each test, theafterEachis unnecessary.Apply this diff to remove the redundant hook:
beforeEach(() => { document.body.innerHTML = ""; }); - - afterEach(() => { - document.body.innerHTML = ""; - });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
libs/subscriptions/src/index.ts(0 hunks)libs/subscriptions/src/repository/queries.test.ts(3 hunks)libs/subscriptions/src/repository/queries.ts(1 hunks)libs/subscriptions/src/repository/service.test.ts(5 hunks)libs/subscriptions/src/repository/service.ts(3 hunks)libs/verified-pages/src/assets/js/select_all.test.ts(1 hunks)libs/verified-pages/src/pages/delete-subscription/index.test.ts(9 hunks)libs/verified-pages/src/pages/delete-subscription/index.ts(3 hunks)libs/verified-pages/src/pages/pending-subscriptions/index.test.ts(5 hunks)libs/verified-pages/src/pages/pending-subscriptions/index.ts(2 hunks)libs/verified-pages/src/pages/subscription-management/index.test.ts(3 hunks)libs/verified-pages/src/pages/subscription-management/index.ts(2 hunks)libs/web-core/src/views/components/body-end-scripts.njk(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- libs/subscriptions/src/index.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
Package.json must include"type": "module"and exports field with proper ESM paths.
Pin all dependency versions to specific versions (e.g.,"express": "5.1.0"), except for peer dependencies.
All test packages must use"test": "vitest run"script. Tests should achieve >80% coverage on business logic.
Files:
package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/assets/js/select_all.test.tslibs/subscriptions/src/repository/queries.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.ts
**/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.ts: Pages are registered through explicit imports inapps/web/src/app.ts. Routes are created based on file names within thepages/directory (e.g.,my-page.tsbecomes/my-page, nested routes via subdirectories).
Page controller files must exportGETand/orPOSTfunctions that accept Express Request and Response, render usingres.render(), and handle form submissions.
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.ts
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/assets/js/select_all.test.tslibs/subscriptions/src/repository/queries.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/assets/js/select_all.test.tslibs/subscriptions/src/repository/queries.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/assets/js/select_all.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/assets/js/select_all.test.tslibs/subscriptions/src/repository/service.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
libs/verified-pages/src/assets/js/select_all.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
libs/verified-pages/src/assets/js/select_all.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.ts : Page controller files must export `GET` and/or `POST` functions that accept Express Request and Response, render using `res.render()`, and handle form submissions.
Applied to files:
libs/verified-pages/src/pages/subscription-management/index.test.ts
🧬 Code graph analysis (8)
libs/verified-pages/src/pages/pending-subscriptions/index.ts (1)
libs/subscriptions/src/repository/service.ts (1)
getAllSubscriptionsByUserId(119-122)
libs/subscriptions/src/repository/queries.ts (1)
libs/subscriptions/src/repository/service.ts (1)
deleteSubscriptionsByIds(157-172)
libs/verified-pages/src/pages/subscription-management/index.ts (1)
libs/subscriptions/src/repository/service.ts (1)
getAllSubscriptionsByUserId(119-122)
libs/verified-pages/src/pages/subscription-management/index.test.ts (1)
libs/verified-pages/src/pages/subscription-management/index.ts (1)
GET(53-53)
libs/subscriptions/src/repository/service.ts (1)
libs/subscriptions/src/repository/queries.ts (5)
findSubscriptionById(42-46)findSubscriptionsWithLocationByUserId(54-62)findSubscriptionsByIds(64-74)findSubscriptionsWithLocationByIds(76-86)deleteSubscriptionsByIds(88-99)
libs/verified-pages/src/pages/delete-subscription/index.ts (1)
libs/subscriptions/src/repository/service.ts (1)
getSubscriptionById(37-39)
libs/subscriptions/src/repository/service.test.ts (2)
libs/subscriptions/src/repository/service.ts (6)
getAllSubscriptionsByUserId(119-122)getCaseSubscriptionsByUserId(124-128)getCourtSubscriptionsByUserId(130-132)validateSubscriptionOwnership(134-146)getSubscriptionDetailsForConfirmation(148-155)deleteSubscriptionsByIds(157-172)libs/subscriptions/src/repository/queries.ts (1)
deleteSubscriptionsByIds(88-99)
libs/subscriptions/src/repository/queries.test.ts (2)
libs/subscriptions/src/repository/queries.ts (4)
findSubscriptionsWithLocationByUserId(54-62)findSubscriptionsByIds(64-74)findSubscriptionsWithLocationByIds(76-86)deleteSubscriptionsByIds(88-99)libs/subscriptions/src/repository/service.ts (1)
deleteSubscriptionsByIds(157-172)
🪛 ast-grep (0.40.0)
libs/verified-pages/src/assets/js/select_all.test.ts
[warning] 14-31: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" data-table="${tableId}" /> <table id="${tableId}"> <tbody> <tr> <td><input type="checkbox" class="row-checkbox" /></td> </tr> <tr> <td><input type="checkbox" class="row-checkbox" /></td> </tr> <tr> <td><input type="checkbox" class="row-checkbox" /></td> </tr> </tbody> </table> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 149-168: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" data-table="table1" /> <table id="table1"> <tbody> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> </tbody> </table> </div> <div> <input type="checkbox" class="select-all-checkbox" data-table="table2" /> <table id="table2"> <tbody> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> </tbody> </table> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 191-195: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" /> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 206-213: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" data-table="empty-table" /> <table id="empty-table"> <tbody></tbody> </table> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 226-226: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div></div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 6-6: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 10-10: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 14-31: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" data-table="${tableId}" /> <table id="${tableId}"> <tbody> <tr> <td><input type="checkbox" class="row-checkbox" /></td> </tr> <tr> <td><input type="checkbox" class="row-checkbox" /></td> </tr> <tr> <td><input type="checkbox" class="row-checkbox" /></td> </tr> </tbody> </table> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 149-168: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" data-table="table1" /> <table id="table1"> <tbody> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> </tbody> </table> </div> <div> <input type="checkbox" class="select-all-checkbox" data-table="table2" /> <table id="table2"> <tbody> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> <tr><td><input type="checkbox" class="row-checkbox" /></td></tr> </tbody> </table> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 191-195: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" /> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 206-213: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div> <input type="checkbox" class="select-all-checkbox" data-table="empty-table" /> <table id="empty-table"> <tbody></tbody> </table> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 226-226: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div></div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Tests
- GitHub Check: Test Changed Packages
🔇 Additional comments (26)
libs/web-core/src/views/components/body-end-scripts.njk (1)
2-2: The change is verified and properly coordinated with the build configuration.The variable rename from
index_jstoweb_jsis correctly implemented. The variable is properly defined inapps/web/vite.build.ts(line 23) as an entry point in the Vite build configuration, and all references have been updated consistently—no remainingindex_jsreferences exist in templates. This aligns with the correspondingweb_csschange inbase-template.njk.package.json (1)
69-70: Proper dependency pinning for jws resolution.The change correctly adds a transitive dependency resolution for
jwsat version 4.0.1, properly pinned to a specific version per coding guidelines. This version matches what ships with[email protected], ensuring compatibility across the JWT-related dependencies.libs/verified-pages/src/pages/delete-subscription/index.test.ts (1)
1-165: LGTM! Test refactoring aligns with API changes.The test suite has been correctly updated to use
getSubscriptionByIdinstead offindSubscriptionById. All mocks, expectations, and test cases remain logically consistent with the previous implementation.libs/verified-pages/src/pages/delete-subscription/index.ts (1)
1-135: LGTM! API refactoring correctly applied.The implementation has been properly updated to use
getSubscriptionByIdinstead offindSubscriptionById. The validation logic, ownership checks, and control flow remain intact.libs/subscriptions/src/repository/queries.test.ts (1)
225-409: LGTM! Comprehensive test coverage for new query functions.The new test suites thoroughly cover:
- Subscription retrieval with location data (by userId and by IDs)
- Subscription retrieval without location data (by IDs)
- Transactional bulk deletion with success, zero deletions, and error scenarios
The transaction mocking pattern correctly simulates Prisma's
$transactioncallback behavior.libs/verified-pages/src/pages/pending-subscriptions/index.test.ts (1)
1-193: LGTM! Tests updated to reflect new API and data shape.The test suite correctly:
- Uses
getAllSubscriptionsByUserIdinstead ofgetSubscriptionsByUserId- Reflects the new subscription data shape with
typeandcourtOrTribunalNamefields- Maintains all existing test scenarios and expectations
libs/subscriptions/src/repository/queries.ts (2)
54-99: LGTM! Well-structured query functions for bulk operations.The new query functions provide:
- Locale-aware subscription retrieval with location details
- Selective field retrieval for ownership validation
- Transactional bulk deletion ensuring atomicity
The use of
$transactionfordeleteSubscriptionsByIdsis appropriate to ensure all deletions succeed or fail together.
88-99: No action required. The function is correctly named and properly imported with an alias in the service layer.The query function is correctly exported as
deleteSubscriptionsByIdsfromqueries.ts(line 88). The service layer imports it with an intentional aliasdeleteSubscriptionsByIdsQuery(service.ts line 6) and wraps it in a service function with the same exported namedeleteSubscriptionsByIds(service.ts line 157). This is a valid architectural pattern—the service layer provides an abstraction over the query layer while maintaining consistent naming conventions.libs/verified-pages/src/pages/subscription-management/index.ts (1)
18-23: LGTM! Improved performance by eliminating N+1 queries.The refactoring removes individual location lookups for each subscription and instead relies on location data included in the subscription response via
getAllSubscriptionsByUserId. This eliminates the previous N+1 query pattern and simplifies the code significantly.The locale parameter is correctly passed to ensure locale-aware data retrieval.
libs/verified-pages/src/pages/subscription-management/index.test.ts (1)
1-94: LGTM! Tests correctly verify locale-aware subscription retrieval.The test suite properly:
- Verifies that
getAllSubscriptionsByUserIdis called with bothuserIdandlocaleparameters- Uses the new subscription data shape with
typeandcourtOrTribunalNamefields- Validates that
locationNameis derived fromcourtOrTribunalNamelibs/subscriptions/src/repository/service.test.ts (6)
1-15: LGTM! Imports are well-organized and follow conventions.The imports correctly use
.jsextension for relative imports as required by ESM with Node.js 'nodenext' module resolution. The test file is properly co-located with the source file.
274-342: Good test coverage for locale-aware subscription retrieval.Tests properly verify:
- English locale mapping
- Welsh locale with fallback when
welshNameis null- Empty subscription handling
- Default locale parameter behavior
344-350: Appropriate placeholder test for unimplemented feature.The test correctly verifies the stub behavior returns an empty array until VIBE-300 is implemented.
411-451: Comprehensive ownership validation tests.Good coverage of:
- User owns all subscriptions
- Mixed ownership (different users)
- Non-existent subscriptions
- Empty array short-circuit (avoids unnecessary DB call)
453-516: LGTM! Tests cover confirmation details retrieval comprehensively.Tests properly verify DTO mapping, empty array handling, and locale-specific name selection.
518-583: Thorough bulk deletion tests with proper coverage.Tests cover:
- Successful transactional deletion
- Empty array validation
- Unauthorized access prevention
- Non-existent subscription handling
- Database error propagation
The test at lines 574-582 covers an edge case where ownership validates but deletion returns 0. This could occur in a race condition scenario where subscriptions are deleted between validation and deletion—good defensive coverage.
libs/subscriptions/src/repository/service.ts (8)
1-15: LGTM! Imports follow conventions.Imports correctly use
.jsextension for ESM compatibility. The aliased importdeleteSubscriptionsByIds as deleteSubscriptionsByIdsQueryavoids naming collision with the exported service function.
37-39: LGTM! Clean delegation to query layer.
41-53: LGTM! Proper ownership validation before deletion.The function correctly validates subscription existence and ownership before performing the delete operation.
106-117: LGTM! Clean DTO mapping with locale support.The helper correctly implements Welsh locale fallback logic and keeps types colocated as per guidelines. The inline type parameter is appropriate for this private helper.
119-132: LGTM! Well-structured subscription retrieval functions.The stub for
getCaseSubscriptionsByUserIdappropriately uses underscore-prefixed parameters for unused arguments and includes a clear comment about VIBE-300.
134-146: LGTM! Robust ownership validation.The function correctly:
- Short-circuits on empty array
- Validates all requested subscriptions exist (count check)
- Validates all subscriptions belong to the user
148-155: LGTM! Clean confirmation details retrieval.The short-circuit for empty arrays prevents unnecessary database calls.
157-172: Solid bulk deletion implementation with proper authorization.The function correctly validates input, checks ownership before deletion, and delegates transactional deletion to the query layer. The authorization flow prevents users from deleting subscriptions they don't own.
libs/verified-pages/src/assets/js/select_all.test.ts (2)
7-7: Static analysis innerHTML warnings are false positives.The static analysis tool flagged
innerHTMLassignments as potential XSS vulnerabilities. However, all assignments in this test file use hardcoded string literals for test setup with no user input or dynamic content. ThetableIdparameter increateTestDOMonly receives hardcoded test values like"test-table". These warnings can be safely ignored.Also applies to: 11-11, 14-31, 149-168, 191-195, 206-213, 227-227
1-234: LGTM! Comprehensive test coverage.The test suite is well-structured and provides thorough coverage of the select-all functionality:
- Select-all checkbox toggling all rows
- Row checkbox interactions updating select-all state (checked, unchecked, indeterminate)
- Independent behavior across multiple tables
- Edge cases (missing data-table attribute, empty tables, no checkboxes)
The implementation follows coding guidelines: co-located test file, Vitest framework, proper
.jsimport extension for ESM compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts (1)
46-117: POST handler flow is robust; consider matching GET’s empty-selection handling in the validation branchThe POST handler’s main branches look solid:
- Auth guard and locale selection mirror GET.
- Missing
confirmre-fetches subscriptions, splits case/court, and re-renders with a validation error and CSRF token."no"clears thebulkUnsubscribesession slice and redirects to/subscription-management."yes"now defensively redirects to/bulk-unsubscribewhenselectedIdsis empty, otherwise deletes by IDs for the current user, clears session state, and redirects to success; failures log and redirect back to selection.- Any other value falls back to
/bulk-unsubscribe.One optional refinement: in the
!confirmbranch, ifselectedIdsis unexpectedly empty (e.g. stale tab after session clear), you could mirror the GET behaviour and redirect straight to/bulk-unsubscribeinstead of callinggetSubscriptionDetailsForConfirmationwith an empty list. That would keep both handlers’ treatment of a missing selection perfectly consistent and avoid an unnecessary service call in that edge case.libs/subscriptions/src/repository/service.ts (5)
37-52: Ensure callers ofgetSubscriptionByIdenforce authorization before exposing data
getSubscriptionByIdis a thin wrapper aroundfindSubscriptionByIdand does not takeuserId, so it cannot itself enforce ownership.removeSubscriptioncorrectly performs an ownership check after fetching, but any other callers ofgetSubscriptionByIdmust do the same before returning details to the client to avoid leaking subscription metadata for other users.You may also want to consider reusing
getSubscriptionByIdinsideremoveSubscription(instead of callingfindSubscriptionByIddirectly) to keep a single access path for “by ID” lookups.
105-140: DTO mapping and locale handling are good; tighten typing and placement of DTO helper
- The
mapSubscriptionToDtoimplementation and Welsh/English name selection look correct given thefindSubscriptionsWithLocation*queries.- If there is any chance the
locationrelation can benullat the Prisma level, consider guarding against that here or tightening the query’swhere/schema; otherwise this will throw at runtime.SubscriptionDtoandmapSubscriptionToDtosit mid-file; per the stated guidelines, you may want to move the interface to the bottom and keep helpers below exported functions for consistency.getAllSubscriptionsByUserId,getCaseSubscriptionsByUserId, andgetCourtSubscriptionsByUserIdwould benefit from explicit return types, e.g.Promise<SubscriptionDto[]>. In particular, the stubbedgetCaseSubscriptionsByUserIdcurrently returns a bare[], which will infer toPromise<never[]>; explicitly typing it and returning[] as SubscriptionDto[]keeps the future implementation drop-in.Example for the stub:
-export async function getCaseSubscriptionsByUserId(_userId: string, _locale = "en") { +export async function getCaseSubscriptionsByUserId(_userId: string, _locale = "en"): Promise<SubscriptionDto[]> { // Case subscriptions not yet implemented (VIBE-300) // When implemented, this will query a case_subscription table - return []; + return [] as SubscriptionDto[]; }
142-154: Ownership validation logic is sound; consider edge cases and minor efficiency tweaksThe ownership validation correctly ensures:
- The caller provided at least one ID.
- All provided IDs exist (
subscriptions.length === subscriptionIds.length).- Every subscription belongs to the requesting user.
This is a solid guard before bulk operations. Two minor considerations:
- If the front-end could ever submit duplicate IDs, this will deliberately fail (since
INreturns unique rows and the lengths will differ). That’s safe but may be surprising; if duplicates are expected from the UI, consider dedupingsubscriptionIdsbefore validation.- Since the subsequent delete call also filters by
userId, you could in future fold “existence + ownership” into a single DB round-trip (e.g., by comparing the delete count withsubscriptionIds.length) to avoid the extra query, though it’s not required for correctness.
156-163: Verify thatgetSubscriptionDetailsForConfirmationis only used after an ownership checkThis helper fetches subscription details purely by ID, without scoping to
userId. That’s fine as long as all call sites first validate ownership (e.g., viavalidateSubscriptionOwnership) before using the returned DTOs to render a confirmation page. Otherwise, a malicious user could probe subscription IDs and see court names they don’t own, even though deletion would later be rejected.Please double-check the callers and either:
- Ensure they always perform an ownership check first, or
- Consider extending this API to take
userIdand delegate to a user-scoped query to make misuse harder.
165-178: Bulk delete flow is correct and avoids PII in logs; consider small cleanupsThe bulk delete implementation has the right security properties:
- Rejects empty input with a clear error.
- Calls
validateSubscriptionOwnershipbefore attempting deletion.- Delegates to
deleteSubscriptionsByIdsQuery, which itself filters byuserId, ensuring no records for other users can be deleted even if the validation were bypassed.- Returns the deleted count and does not log
userIdor other PII (which addresses the prior logging concern).Minor cleanup opportunity:
- const count = await deleteSubscriptionsByIdsQuery(subscriptionIds, userId); - - return count; + return deleteSubscriptionsByIdsQuery(subscriptionIds, userId);Functionally identical, just a bit leaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/subscriptions/src/repository/service.ts(3 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/subscriptions/src/repository/service.ts
**/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.ts: Pages are registered through explicit imports inapps/web/src/app.ts. Routes are created based on file names within thepages/directory (e.g.,my-page.tsbecomes/my-page, nested routes via subdirectories).
Page controller files must exportGETand/orPOSTfunctions that accept Express Request and Response, render usingres.render(), and handle form submissions.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/subscriptions/src/repository/service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.tslibs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.tslibs/subscriptions/src/repository/service.ts
🧠 Learnings (2)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.ts : Page controller files must export `GET` and/or `POST` functions that accept Express Request and Response, render using `res.render()`, and handle form submissions.
Applied to files:
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts
🧬 Code graph analysis (2)
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts (3)
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts (2)
GET(119-119)POST(120-120)libs/verified-pages/src/pages/bulk-unsubscribe-success/index.ts (1)
GET(26-26)libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (2)
GET(161-161)POST(162-162)
libs/subscriptions/src/repository/service.ts (1)
libs/subscriptions/src/repository/queries.ts (5)
findSubscriptionById(42-46)findSubscriptionsWithLocationByUserId(54-62)findSubscriptionsByIds(64-74)findSubscriptionsWithLocationByIds(76-86)deleteSubscriptionsByIds(88-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (4)
libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts (2)
44-147: GET handler tests comprehensively cover success, guard, and error pathsThe GET suite exercises the key scenarios (happy-path with case/court split, missing/empty session selections, unauthenticated user, and service failure) and asserts the right render/redirect behaviour and payload structure. This aligns well with the controller logic; no changes needed here.
149-268: POST handler tests validate all key decision branches and session behaviourThe POST suite neatly covers
confirm: "yes","no", and missing cases, including deletion success, deletion failure, session-missing/empty selectedIds guards, and unauthenticated redirects. The assertions on deletion calls, redirects, and session clearing match the controller’s intent and give solid confidence in the flow.libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts (1)
7-44: GET handler correctly enforces auth, selection guard, and case/court splittingThe GET handler now:
- Short-circuits unauthenticated users to
/sign-in.- Reads
selectedIdsfromsession.bulkUnsubscribeand redirects back to/bulk-unsubscribewhen missing/empty.- Fetches confirmation details and cleanly splits them into
caseSubscriptionsandcourtSubscriptionswith corresponding boolean flags.- Ensures
res.locals.navigation.verifiedItemsis populated and renders the view with the correct locale bundle and CSRF token.
This resolves the earlier concerns about case/court handling and makes the read path robust.libs/subscriptions/src/repository/service.ts (1)
6-12: Imports and aliasing of deleteSubscriptionsByIds look consistentThe additional query imports and the alias
deleteSubscriptionsByIdsQueryare consistent withqueries.tsand avoid naming clashes with the new service-leveldeleteSubscriptionsByIdsfunction. No issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (3)
49-49: Avoidanytype cast for CSRF token retrieval.The
(req as any).csrfToken?.()pattern is used in multiple places (lines 49, 70, 114). Per coding guidelines,anyshould not be used without justification.Consider extending the Express Request type or creating a typed helper:
+interface CsrfRequest extends Request { + csrfToken?: () => string; +} + const getHandler = async (req: Request, res: Response) => {Then use
(req as CsrfRequest).csrfToken?.() || ""for type-safe access, or extract a helper function:const getCsrfToken = (req: Request): string => (req as CsrfRequest).csrfToken?.() ?? "";
31-34: Consider extracting navigation setup to reduce duplication.Navigation initialization appears three times (lines 31-34, 54-57, 92-95). This could be extracted into a helper.
const ensureNavigation = (res: Response, path: string, locale: string) => { if (!res.locals.navigation) { res.locals.navigation = {}; } res.locals.navigation.verifiedItems = buildVerifiedUserNavigation(path, locale); };
83-83: Consider simplifying the subscription parsing logic.The nested ternary is functional but less readable.
- const selectedIds = Array.isArray(req.body.subscriptions) ? req.body.subscriptions : req.body.subscriptions ? [req.body.subscriptions] : []; + const rawSubscriptions = req.body.subscriptions; + const selectedIds: string[] = Array.isArray(rawSubscriptions) + ? rawSubscriptions + : rawSubscriptions + ? [rawSubscriptions] + : [];Or use a more explicit approach:
const selectedIds: string[] = [req.body.subscriptions].flat().filter(Boolean);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/verified-pages/src/pages/bulk-unsubscribe/index.njk(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/index.test.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/verified-pages/src/pages/bulk-unsubscribe/index.test.ts
- libs/verified-pages/src/pages/bulk-unsubscribe/index.njk
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts
**/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.ts: Pages are registered through explicit imports inapps/web/src/app.ts. Routes are created based on file names within thepages/directory (e.g.,my-page.tsbecomes/my-page, nested routes via subdirectories).
Page controller files must exportGETand/orPOSTfunctions that accept Express Request and Response, render usingres.render(), and handle form submissions.
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts
🧠 Learnings (1)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.ts : Page controller files must export `GET` and/or `POST` functions that accept Express Request and Response, render using `res.render()`, and handle form submissions.
Applied to files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (4)
libs/verified-pages/src/pages/bulk-unsubscribe/index.ts (4)
1-5: LGTM!Imports correctly use workspace aliases for package imports and
.jsextensions for local files, following ESM conventions.
7-15: LGTM!Interface naming follows PascalCase convention, and the session augmentation pattern for Express is correctly implemented.
85-120: Validation error path correctly re-renders with fresh data.The approach of re-fetching subscriptions when validation fails ensures the user sees up-to-date data. The error structure with
textandhrefmatches GOV.UK error summary patterns.
130-131: LGTM!Exports correctly follow the page controller pattern with appropriate middleware chain for authentication and access control. Based on learnings, page controllers should export
GETand/orPOSThandlers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libs/verified-pages/src/pages/delete-subscription/index.test.ts (1)
97-155: Consider adding a POST test for unauthorized subscription access.The GET tests cover the case when
getSubscriptionByIdreturnsnull, but the POST tests all mock successful subscription lookups. Consider adding a test case for when a user attempts to POST with a subscription they don't own.Example test case:
it("should redirect to subscription-management if user does not own subscription", async () => { const validSubscriptionId = "550e8400-e29b-41d4-a716-446655440000"; mockReq.body = { subscription: validSubscriptionId, "unsubscribe-confirm": "yes" }; vi.mocked(subscriptionService.getSubscriptionById).mockResolvedValue(null); await POST[POST.length - 1](mockReq as Request, mockRes as Response, vi.fn()); expect(mockRes.redirect).toHaveBeenCalledWith("/subscription-management"); });libs/subscriptions/src/repository/queries.ts (1)
91-101: Transaction wrapper may be unnecessary for a single operation.The
$transactionwrapper contains only a singledeleteManyoperation. Prisma'sdeleteManyis already atomic for a single statement. The transaction adds overhead but provides no additional guarantee here.However, if this is intentional for consistency with other patterns or for future extension (e.g., adding audit logging within the transaction), it's acceptable.
If atomicity for a single operation is sufficient, consider simplifying:
export async function deleteSubscriptionsByIds(subscriptionIds: string[], userId: string) { - return prisma.$transaction(async (tx) => { - const deleteResult = await tx.subscription.deleteMany({ - where: { - subscriptionId: { in: subscriptionIds }, - userId - } - }); - - return deleteResult.count; - }); + const deleteResult = await prisma.subscription.deleteMany({ + where: { + subscriptionId: { in: subscriptionIds }, + userId + } + }); + + return deleteResult.count; }libs/subscriptions/src/repository/queries.test.ts (1)
364-385: Transaction mock could verify the where clause parameters.The mock transaction setup correctly simulates the callback pattern, but it doesn't verify that
deleteManyis called with the correctwhereclause (subscription IDs and userId). Consider adding an assertion to validate the call arguments.it("should delete multiple subscriptions in a transaction", async () => { const subscriptionIds = ["sub-1", "sub-2"]; const userId = "user123"; const mockDeleteResult = { count: 2 }; + const mockDeleteMany = vi.fn().mockResolvedValue(mockDeleteResult); const mockTransaction = vi.fn(async (callback) => { const tx = { subscription: { - deleteMany: vi.fn().mockResolvedValue(mockDeleteResult) + deleteMany: mockDeleteMany } }; return callback(tx); }); vi.mocked(prisma.$transaction).mockImplementation(mockTransaction); const result = await deleteSubscriptionsByIds(subscriptionIds, userId); expect(result).toBe(2); expect(prisma.$transaction).toHaveBeenCalled(); + expect(mockDeleteMany).toHaveBeenCalledWith({ + where: { + subscriptionId: { in: subscriptionIds }, + userId + } + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/subscriptions/src/repository/queries.test.ts(3 hunks)libs/subscriptions/src/repository/queries.ts(1 hunks)libs/subscriptions/src/repository/service.test.ts(6 hunks)libs/subscriptions/src/repository/service.ts(4 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts(1 hunks)libs/verified-pages/src/pages/delete-subscription/index.test.ts(9 hunks)libs/verified-pages/src/pages/delete-subscription/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/verified-pages/src/pages/delete-subscription/index.ts
- libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.test.ts
- libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/subscriptions/src/repository/queries.tslibs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/subscriptions/src/repository/service.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/subscriptions/src/repository/queries.tslibs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/subscriptions/src/repository/service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/subscriptions/src/repository/queries.tslibs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/subscriptions/src/repository/service.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/subscriptions/src/repository/service.test.tslibs/subscriptions/src/repository/queries.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.ts
**/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.ts: Pages are registered through explicit imports inapps/web/src/app.ts. Routes are created based on file names within thepages/directory (e.g.,my-page.tsbecomes/my-page, nested routes via subdirectories).
Page controller files must exportGETand/orPOSTfunctions that accept Express Request and Response, render usingres.render(), and handle form submissions.
Files:
libs/verified-pages/src/pages/delete-subscription/index.test.ts
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/verified-pages/src/pages/delete-subscription/index.test.ts
🧠 Learnings (1)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.ts
🧬 Code graph analysis (3)
libs/subscriptions/src/repository/queries.ts (1)
libs/subscriptions/src/repository/service.ts (1)
deleteSubscriptionsByIds(152-164)
libs/subscriptions/src/repository/queries.test.ts (2)
libs/subscriptions/src/repository/queries.ts (6)
findSubscriptionById(42-46)deleteSubscriptionRecord(48-53)findSubscriptionsWithLocationByUserId(55-63)findSubscriptionsByIds(65-76)findSubscriptionsWithLocationByIds(78-89)deleteSubscriptionsByIds(91-102)libs/subscriptions/src/repository/service.ts (1)
deleteSubscriptionsByIds(152-164)
libs/subscriptions/src/repository/service.ts (1)
libs/subscriptions/src/repository/queries.ts (5)
findSubscriptionById(42-46)deleteSubscriptionRecord(48-53)findSubscriptionsWithLocationByUserId(55-63)findSubscriptionsWithLocationByIds(78-89)deleteSubscriptionsByIds(91-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (18)
libs/verified-pages/src/pages/delete-subscription/index.test.ts (2)
1-14: LGTM! Clean mock setup aligned with new API.The mock structure correctly reflects the updated
getSubscriptionByIdfunction signature that now requires bothsubscriptionIdanduserIdfor ownership validation. Import and mock setup are well-structured.
56-64: Good consolidation of ownership check scenarios.Combining "subscription not found" and "user doesn't own subscription" into a single test case is appropriate—the service layer returns
nullfor both, and the controller correctly handles this uniformly.libs/subscriptions/src/repository/service.test.ts (4)
4-14: LGTM! Imports correctly updated for new API surface.The imports correctly include all the new service functions being tested, and use the
.jsextension as required by ESM with Node.js 'nodenext' module resolution per coding guidelines.
79-123: Well-structured tests for ownership-scoped deletion.The tests correctly validate the updated
removeSubscriptionflow:
- Lookup with ownership check via
findSubscriptionById(subscriptionId, userId)- Deletion with ownership constraint via
deleteSubscriptionRecord(subscriptionId, userId)- Proper error handling when subscription not found or delete fails
332-339: Good coverage for Welsh localization with fallback.The test correctly validates the fallback behavior: Welsh name is used when available (
"Welsh Birmingham Crown Court"), and English name is used when Welsh isnull("Manchester Crown Court"). This ensures proper bilingual support per coding guidelines.
482-525: Comprehensive test coverage for bulk deletion with authorization.The tests thoroughly validate:
- Successful bulk deletion returns correct count
- Empty input validation
- Ownership validation via count mismatch detection
- Database error propagation
- Zero-match scenario handling
This ensures the transactional bulk delete is robust and secure.
libs/subscriptions/src/repository/queries.ts (3)
42-46: LGTM! Ownership-scoped lookup is secure.Using
findFirstwith{ subscriptionId, userId }ensures that subscriptions can only be accessed by their owner. This is a proper security pattern.
48-53: Good use of deleteMany for ownership-safe deletion.Using
deleteManyinstead ofdeleteis appropriate here—it returns a count (enabling validation) and doesn't throw when no records match. TheuserIdin the where clause ensures users can only delete their own subscriptions.
55-63: LGTM! Location-enriched query is well-structured.The query correctly includes the location relation for DTO mapping and maintains consistent ordering by
dateAddeddescending.libs/subscriptions/src/repository/service.ts (6)
1-14: LGTM! Imports correctly structured with.jsextensions.The imports follow ESM conventions with
.jsextensions and use theasalias fordeleteSubscriptionsByIdsto avoid naming collision with the local service function.
36-38: LGTM! Clean public API wrapper.The function provides a simple public interface delegating to the query layer where the ownership check is enforced.
40-54: Good defensive approach with dual validation.The function validates ownership twice: once during lookup and again by checking the delete count. This handles the race condition where a subscription might be deleted between the lookup and delete operations.
133-137: LGTM! Clear placeholder for future implementation.The underscore-prefixed parameters and comment clearly indicate this is a placeholder for VIBE-300 case subscriptions work.
152-164: LGTM! Secure bulk deletion with proper authorization.The implementation correctly:
- Validates non-empty input upfront
- Delegates to the transactional query
- Validates the deletion count matches the request count to ensure ownership
The authorization error message is appropriately generic ("User does not own all selected subscriptions") without leaking which specific subscriptions were unauthorized.
107-126: Good internal helper with proper Welsh localization.The
mapSubscriptionToDtofunction correctly:
- Remains unexported (internal use only, per guidelines)
- Handles Welsh locale with fallback to English when
welshNameis null- Has the
SubscriptionDtointerface colocated rather than in a separatetypes.ts- Follows module ordering with the interface placed at the bottom
The
SubscriptionDtointerface is only used internally as the return type of this private function and requires no export.libs/subscriptions/src/repository/queries.test.ts (3)
16-29: LGTM! Comprehensive Prisma mock setup.The mock correctly includes all Prisma methods used by the queries:
findFirst,deleteMany, and$transactionfor the new ownership-scoped operations.
122-153: LGTM! Tests correctly validate ownership-scoped lookup.The tests properly verify that
findFirstis called with bothsubscriptionIdanduserIdin the where clause, ensuring the ownership check is enforced at the query level.
207-232: Good coverage for ownership-scoped deletion.Tests correctly validate:
deleteManyis called with ownership constraint- Count is returned for caller validation
- Zero count when subscription doesn't exist or isn't owned
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libs/subscriptions/src/repository/service.ts (3)
40-54: Redundant prefetch before delete – could be simplified.
removeSubscriptionnow correctly scopes both the lookup and delete touserId, which fixes ownership concerns. However, the initialfindSubscriptionByIdcall is redundant becausedeleteSubscriptionRecord(subscriptionId, userId)already returns a count that you check for zero and re-throw as “Subscription not found”. You could drop the prefetch and rely solely on the delete count to reduce one DB round‑trip:-export async function removeSubscription(subscriptionId: string, userId: string) { - const subscription = await findSubscriptionById(subscriptionId, userId); - - if (!subscription) { - throw new Error("Subscription not found"); - } - - const count = await deleteSubscriptionRecord(subscriptionId, userId); - - if (count === 0) { - throw new Error("Subscription not found"); - } - - return count; -} +export async function removeSubscription(subscriptionId: string, userId: string) { + const count = await deleteSubscriptionRecord(subscriptionId, userId); + + if (count === 0) { + throw new Error("Subscription not found"); + } + + return count; +}
107-126: DTO mapping is sound; consider tightening types and placement.The
mapSubscriptionToDtoimplementation matches the shape returned by thefindSubscriptionsWithLocation*queries and correctly handles Welsh names whenlocale === "cy". Two small polish points:
- You could constrain
localeto a union like"en" | "cy"to avoid accidental typos propagating through.- Per the module ordering guidelines, consider moving the
SubscriptionDtointerface to the bottom of the file, below the exported functions, while keepingmapSubscriptionToDtonear its callers.As per coding guidelines, keeping types at the bottom and constraining string enums improves clarity and maintainability.
143-150: Confirmation details helper is correctly scoped and guarded.
getSubscriptionDetailsForConfirmationcorrectly:
- Returns early on an empty
subscriptionIdsarray.- Fetches only subscriptions owned by
userIdviafindSubscriptionsWithLocationByIds.- Maps to DTOs using the same locale-aware logic.
One optional enhancement: you might want to assert that all requested IDs are found (e.g., compare
subscriptions.lengthto the number of uniquesubscriptionIds) and treat mismatches as an error to catch stale or tampered IDs earlier in the flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/subscriptions/src/repository/service.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/subscriptions/src/repository/service.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/subscriptions/src/repository/service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/subscriptions/src/repository/service.ts
🧬 Code graph analysis (1)
libs/subscriptions/src/repository/service.ts (1)
libs/subscriptions/src/repository/queries.ts (5)
findSubscriptionById(42-46)deleteSubscriptionRecord(48-53)findSubscriptionsWithLocationByUserId(55-63)findSubscriptionsWithLocationByIds(78-89)deleteSubscriptionsByIds(91-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (4)
libs/subscriptions/src/repository/service.ts (4)
1-12: Imports for new query helpers look consistent and aligned with user-scoped access.The added imports/aliasing for
deleteSubscriptionsByIdsQuery,findSubscriptionById, and thefindSubscriptions*WithLocation*helpers are consistent with the existing style and correctly point to the query layer; no issues here.
36-38: User-scopedgetSubscriptionByIdwrapper is clear and safe.Thinly wrapping
findSubscriptionById(subscriptionId, userId)keeps lookups correctly scoped to the current user without extra logic. This is a sensible addition to the service surface.
69-105: User-scoped deletion inreplaceUserSubscriptionslooks correct.Updating the deletion branch to call
deleteSubscriptionRecord(sub.subscriptionId, userId)ensures this bulk replace flow cannot accidentally touch another user’s subscriptions, while preserving the existing validation and add/delete behaviour.
128-141: New subscription retrieval APIs are coherent and locale-aware.
getAllSubscriptionsByUserIdcorrectly delegates tofindSubscriptionsWithLocationByUserIdand maps viamapSubscriptionToDto, giving you locale-aware names.getCaseSubscriptionsByUserIdsafely returns an empty array for now, which is an appropriate non-breaking placeholder until VIBE‑300 is delivered.getCourtSubscriptionsByUserIdis a clear wrapper aroundgetAllSubscriptionsByUserId, which keeps the API ergonomic for callers that only care about court subscriptions.These additions fit well with the DTO and query-layer contracts.
| export async function deleteSubscriptionsByIds(subscriptionIds: string[], userId: string) { | ||
| if (subscriptionIds.length === 0) { | ||
| throw new Error("No subscriptions provided for deletion"); | ||
| } | ||
|
|
||
| const count = await deleteSubscriptionsByIdsQuery(subscriptionIds, userId); | ||
|
|
||
| if (count !== subscriptionIds.length) { | ||
| throw new Error("Unauthorized: User does not own all selected subscriptions"); | ||
| } | ||
|
|
||
| return count; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the deleteSubscriptionsByIds function and related tests
rg -n "deleteSubscriptionsByIds" --type ts -B 3 -A 10Repository: hmcts/cath-service
Length of output: 25933
🏁 Script executed:
# Find the deleteSubscriptionsByIdsQuery implementation to understand how it works
rg -n "deleteSubscriptionsByIdsQuery" --type ts -B 2 -A 15Repository: hmcts/cath-service
Length of output: 2198
🏁 Script executed:
# Search for tests related to delete subscriptions
fd -e test.ts -e spec.ts | xargs rg -l "deleteSubscriptions" 2>/dev/nullRepository: hmcts/cath-service
Length of output: 230
🏁 Script executed:
# Check the Prisma version and transaction behavior
cat libs/subscriptions/package.json | grep -A 5 '"@prisma'Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Verify the actual flow - check how deleteMany handles duplicates and if there are any other calls to deleteSubscriptionsByIdsQuery
rg -n "deleteSubscriptionsByIdsQuery\|deleteSubscriptionsByIds" --type ts -B 2 -A 2 | head -100Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check if there's any client-side deduplication before calling the function
rg -n "selectedIds" --type ts -B 3 -A 3Repository: hmcts/cath-service
Length of output: 11379
Add deduplication and pre-check for safer bulk delete semantics.
The ownership check via count !== subscriptionIds.length works but has edge cases:
- Duplicates: If
subscriptionIdscontains duplicates,deleteManyonly deletes each row once. While duplicates are unlikely from the form checkbox UI, they could arrive from other callers. The PrismaINoperator naturally de-duplicates, but comparing against the original array length will fail unexpectedly. - Partial deletion is committed:
deleteManyexecutes and commits before the service layer validates the count. If the user owns only some subscriptions, those rows are deleted, then an error is thrown. This means the error message ("Unauthorized") is misleading—the owned subscriptions are already gone.
To make this more robust, deduplicate IDs before querying and consider pre-validating ownership to avoid partial deletes:
export async function deleteSubscriptionsByIds(subscriptionIds: string[], userId: string) {
if (subscriptionIds.length === 0) {
throw new Error("No subscriptions provided for deletion");
}
const uniqueIds = [...new Set(subscriptionIds)];
const count = await deleteSubscriptionsByIdsQuery(uniqueIds, userId);
if (count !== uniqueIds.length) {
throw new Error("Unauthorized: User does not own all selected subscriptions");
}
return count;
}Optionally add a pre-check to fail before any deletions:
// const owned = await findSubscriptionsWithLocationByIds(uniqueIds, userId);
// if (owned.length !== uniqueIds.length) {
// throw new Error("Unauthorized: User does not own all selected subscriptions");
// }🤖 Prompt for AI Agents
In libs/subscriptions/src/repository/service.ts around lines 152 to 164,
deduplicate the incoming subscriptionIds and perform an ownership pre-check
before calling deleteSubscriptionsByIdsQuery to avoid partial commits and
duplicate-count mismatches: create uniqueIds from subscriptionIds, query for
subscriptions owned by user with those uniqueIds and throw the Unauthorized
error if the found count does not equal uniqueIds.length, then call
deleteSubscriptionsByIdsQuery with uniqueIds and return the delete count
(compare against uniqueIds.length if you want an additional safeguard).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
libs/subscriptions/src/repository/queries.ts (2)
42-46: Consider enforcing a DB constraint/index for(subscriptionId, userId)(then preferfindUnique).
findFirst({ where: { subscriptionId, userId } })is correct for ownership scoping, but performance hinges on proper indexing. If you can add a composite unique/index on(subscriptionId, userId), switching tofindUniquemakes the intent + query plan clearer.
78-89: Transaction wrapper is probably redundant; also guard caller-side length checks against duplicate IDs.
- If this is only one
deleteMany, you can likely drop$transactionunless you’re planning to add more steps.- In
libs/subscriptions/src/repository/service.ts(snippet showscount !== subscriptionIds.length), duplicates insubscriptionIdscan cause a false “Unauthorized” throw. Consider deduping before delete + compare.Suggested service-layer hardening (example):
export async function deleteSubscriptionsByIds(subscriptionIds: string[], userId: string) { if (subscriptionIds.length === 0) { throw new Error("No subscriptions provided for deletion"); } + const uniqueIds = Array.from(new Set(subscriptionIds)); - const count = await deleteSubscriptionsByIdsQuery(subscriptionIds, userId); + const count = await deleteSubscriptionsByIdsQuery(uniqueIds, userId); - if (count !== subscriptionIds.length) { + if (count !== uniqueIds.length) { throw new Error("Unauthorized: User does not own all selected subscriptions"); } return count; }libs/subscriptions/src/repository/queries.test.ts (1)
334-389: StrengthendeleteSubscriptionsByIdstests by asserting the innertx.subscription.deleteManycall args.
Right now the test only checks$transactionwas called; it doesn’t verify thewhere: { subscriptionId: { in: ... }, userId }filter is applied.Example adjustment:
it("should delete multiple subscriptions in a transaction", async () => { const subscriptionIds = ["sub-1", "sub-2"]; const userId = "user123"; const mockDeleteResult = { count: 2 }; + const deleteManySpy = vi.fn().mockResolvedValue(mockDeleteResult); const mockTransaction = vi.fn(async (callback) => { const tx = { subscription: { - deleteMany: vi.fn().mockResolvedValue(mockDeleteResult) + deleteMany: deleteManySpy } }; return callback(tx); }); vi.mocked(prisma.$transaction).mockImplementation(mockTransaction); const result = await deleteSubscriptionsByIds(subscriptionIds, userId); expect(result).toBe(2); expect(prisma.$transaction).toHaveBeenCalled(); + expect(deleteManySpy).toHaveBeenCalledWith({ + where: { subscriptionId: { in: subscriptionIds }, userId } + }); });e2e-tests/tests/bulk-unsubscribe.spec.ts (1)
93-113: Use console.error for error logging in cleanup.The cleanup function uses
console.logfor error reporting, butconsole.errorwould be more appropriate for error cases.Apply this diff:
} catch (error) { - console.log("Test data cleanup:", error); + console.error("Test data cleanup:", error); }libs/verified-pages/src/pages/bulk-unsubscribe/index.njk (1)
35-38: Consider extracting inline styles to CSS classes.Inline
style="vertical-align: middle;"is used throughout the template on table headers and cells. While this works, it would be cleaner to use CSS classes for maintainability and consistency.Consider adding a CSS class in your stylesheet:
.govuk-table__header--middle, .govuk-table__cell--middle { vertical-align: middle; }Then apply the class instead:
-<th scope="col" class="govuk-table__header" style="vertical-align: middle;"> +<th scope="col" class="govuk-table__header govuk-table__header--middle">Note: Check if GOV.UK Design System already provides such utilities before adding custom classes.
Also applies to: 60-61, 89-90, 109-111, 138-142, 160-164, 193-195, 213-215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e-tests/tests/bulk-unsubscribe.spec.ts(1 hunks)libs/subscriptions/src/repository/queries.test.ts(3 hunks)libs/subscriptions/src/repository/queries.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/index.njk(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/subscriptions/src/repository/queries.tse2e-tests/tests/bulk-unsubscribe.spec.tslibs/subscriptions/src/repository/queries.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/subscriptions/src/repository/queries.tse2e-tests/tests/bulk-unsubscribe.spec.tslibs/subscriptions/src/repository/queries.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/subscriptions/src/repository/queries.tse2e-tests/tests/bulk-unsubscribe.spec.tslibs/subscriptions/src/repository/queries.test.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/bulk-unsubscribe.spec.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/subscriptions/src/repository/queries.test.ts
**/src/pages/**/*.njk
📄 CodeRabbit inference engine (CLAUDE.md)
Nunjucks templates must extend
layouts/base-templates.njk, use govuk macros for components, include error summaries, and support conditional rendering based on language variables.
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.njk
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.njk
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.{ts,tsx} : Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Applied to files:
libs/subscriptions/src/repository/queries.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/bulk-unsubscribe.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/bulk-unsubscribe.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk`, use govuk macros for components, include error summaries, and support conditional rendering based on language variables.
Applied to files:
libs/verified-pages/src/pages/bulk-unsubscribe/index.njk
🧬 Code graph analysis (2)
libs/subscriptions/src/repository/queries.ts (1)
libs/subscriptions/src/repository/service.ts (1)
deleteSubscriptionsByIds(152-164)
e2e-tests/tests/bulk-unsubscribe.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
🪛 ast-grep (0.40.0)
e2e-tests/tests/bulk-unsubscribe.spec.ts
[warning] 253-253: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(Select ${testData.locationName1})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 254-254: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(Select ${testData.locationName2})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 255-255: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(Select ${testData.locationName3})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (15)
libs/subscriptions/src/repository/queries.ts (2)
48-53:deleteSubscriptionRecordreturningcountis a solid, non-throwing ownership-safe delete.
UsingdeleteManywith{ subscriptionId, userId }avoids leaking whether a subscription exists for another user.
55-76: Location-including query helpers look consistent and UI-friendly.
Ordering bydateAdded: "desc"andinclude: { location: true }matches the bulk-unsubscribe UI needs.libs/subscriptions/src/repository/queries.test.ts (4)
15-28: Prisma mock additions align with the new query implementations.
findFirst,deleteMany, and$transactioncoverage looks in sync with the updated queries.
121-151: UpdatedfindSubscriptionByIdtests correctly assert the user-scoped lookup.
Nice to see the assertion explicitly checks{ subscriptionId, userId }is passed tofindFirst.
206-231:deleteSubscriptionRecordtests cover both owned and not-owned/not-found cases.
Returning0in the latter case matches the ownership-safe delete semantics.
233-332: Location-enriched query tests are clear and verify ordering/include args.
These assertions should protect the bulk-unsubscribe UI from regressing on the shape of returned data.e2e-tests/tests/bulk-unsubscribe.spec.ts (7)
1-18: LGTM! Well-structured test setup.The imports, interface, and test data map provide good isolation between tests. The use of ESM imports with
.jsextensions is correct per coding guidelines.
20-91: Test data creation is well-implemented with one minor consideration.The function generates unique test data and uses upsert to handle potential conflicts. The database query for existing sub-jurisdiction and region (lines 40-45) assumes a seeded database, which is reasonable for E2E tests.
115-142: Excellent test isolation with beforeEach/afterEach hooks.The test setup creates fresh data per test and ensures cleanup, providing good isolation. The CFT IDAM login flow is properly abstracted.
143-267: Comprehensive test coverage of the bulk unsubscribe flow.This section thoroughly tests the core functionality including:
- Subscription creation
- Page navigation and structure
- Tab navigation and keyboard controls
- Welsh translation support
- Validation with no selections
- Select-all functionality
The test methodically validates each step of the user journey.
194-197: Accessibility checks appropriately implemented.The test uses Axe-core for WCAG compliance checks and disables the "region" rule, which is a common practice when the page structure doesn't require landmark regions. The accessibility scans at multiple stages (selection, confirmation, success) ensure comprehensive coverage.
Also applies to: 300-303, 352-355
253-256: Static analysis false positive - RegExp from controlled test data is safe.The static analysis tool warns about ReDoS risk from constructing RegExp with variable input. However, this is a false positive because:
- The test data is generated within the test itself using timestamps and random numbers
- The location names follow a predictable, safe pattern:
E2E Bulk Test Location N ${timestamp}-${random}- There is no user input or external data involved
The RegExp usage here is safe and appropriate for matching the test data in Playwright locators.
268-382: Excellent verification of database state post-flow.The test goes beyond UI validation to verify the actual database state (lines 361-379), confirming that:
- Selected subscriptions were deleted
- Non-selected subscriptions remain intact
This level of verification ensures the feature works end-to-end, not just in the UI. The test also validates Welsh translation on the success page and includes proper accessibility checks throughout.
libs/verified-pages/src/pages/bulk-unsubscribe/index.njk (2)
1-19: LGTM! Template setup follows conventions.The template correctly extends the base template, imports necessary GOV.UK macros, includes page title, and displays error summary when needed.
239-277: Tab implementation and script loading look good.The govukTabs macro properly handles client-side tab switching, and the select-all JavaScript is appropriately loaded at the end. The tab labels include counts for better UX.
| {% set allTabHtml %} | ||
| {% if hasCaseSubscriptions %} | ||
| <h2 class="govuk-heading-m">{{ tabSubscriptionsByCase }}</h2> | ||
| <table class="govuk-table" id="case-subscriptions-table"> | ||
| <thead class="govuk-table__head"> | ||
| <tr class="govuk-table__row"> | ||
| <th scope="col" class="govuk-table__header" style="vertical-align: middle;">{{ tableHeaderCaseName }}</th> | ||
| <th scope="col" class="govuk-table__header" style="vertical-align: middle;">{{ tableHeaderPartyName }}</th> | ||
| <th scope="col" class="govuk-table__header" style="vertical-align: middle;">{{ tableHeaderReferenceNumber }}</th> | ||
| <th scope="col" class="govuk-table__header" style="vertical-align: middle;">{{ tableHeaderDateAdded }}</th> | ||
| <th scope="col" class="govuk-table__header govuk-table__header--numeric" style="vertical-align: middle;"> | ||
| <div class="govuk-checkboxes__item govuk-checkboxes--small"> | ||
| <input | ||
| class="govuk-checkboxes__input select-all-checkbox" | ||
| id="select-all-case" | ||
| type="checkbox" | ||
| data-table="case-subscriptions-table" | ||
| aria-label="Select all case subscriptions"> | ||
| <label class="govuk-label govuk-checkboxes__label" for="select-all-case"> | ||
| <span class="govuk-visually-hidden">Select all</span> | ||
| </label> | ||
| </div> | ||
| </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 }}</td> | ||
| <td class="govuk-table__cell" style="vertical-align: middle;">{{ subscription.partyName }}</td> | ||
| <td class="govuk-table__cell" style="vertical-align: middle;">{{ subscription.referenceNumber }}</td> | ||
| <td class="govuk-table__cell" style="vertical-align: middle;">{{ subscription.dateAdded | date('D MMMM YYYY') }}</td> | ||
| <td class="govuk-table__cell govuk-table__cell--numeric" style="vertical-align: middle;"> | ||
| <div class="govuk-checkboxes__item govuk-checkboxes--small"> | ||
| <input | ||
| class="govuk-checkboxes__input row-checkbox" | ||
| id="case-{{ subscription.subscriptionId }}" | ||
| name="subscriptions" | ||
| type="checkbox" | ||
| value="{{ subscription.subscriptionId }}" | ||
| aria-label="Select {{ subscription.caseName }}" | ||
| {% if subscription.subscriptionId in previouslySelected %}checked{% endif %}> | ||
| <label class="govuk-label govuk-checkboxes__label" for="case-{{ subscription.subscriptionId }}"> | ||
| <span class="govuk-visually-hidden">Select {{ subscription.caseName }}</span> | ||
| </label> | ||
| </div> | ||
| </td> | ||
| </tr> | ||
| {% endfor %} | ||
| </tbody> | ||
| </table> | ||
| {% endif %} | ||
|
|
||
| {% if hasCourtSubscriptions %} | ||
| {% if hasCaseSubscriptions %} | ||
| <h2 class="govuk-heading-m govuk-!-margin-top-6">{{ tabSubscriptionsByCourt }}</h2> | ||
| {% endif %} | ||
| <table class="govuk-table" id="court-subscriptions-table"> | ||
| <thead class="govuk-table__head"> | ||
| <tr class="govuk-table__row"> | ||
| <th scope="col" class="govuk-table__header" style="vertical-align: middle;">{{ tableHeaderCourtName }}</th> | ||
| <th scope="col" class="govuk-table__header" style="vertical-align: middle;">{{ tableHeaderDateAdded }}</th> | ||
| <th scope="col" class="govuk-table__header govuk-table__header--numeric" style="vertical-align: middle;"> | ||
| <div class="govuk-checkboxes__item govuk-checkboxes--small"> | ||
| <input | ||
| class="govuk-checkboxes__input select-all-checkbox" | ||
| id="select-all-court" | ||
| type="checkbox" | ||
| data-table="court-subscriptions-table" | ||
| aria-label="Select all court subscriptions"> | ||
| <label class="govuk-label govuk-checkboxes__label" for="select-all-court"> | ||
| <span class="govuk-visually-hidden">Select all</span> | ||
| </label> | ||
| </div> | ||
| </th> | ||
| </tr> | ||
| </thead> | ||
| <tbody class="govuk-table__body"> | ||
| {% for subscription in courtSubscriptions %} | ||
| <tr class="govuk-table__row"> | ||
| <td class="govuk-table__cell" style="vertical-align: middle;">{{ subscription.courtOrTribunalName }}</td> | ||
| <td class="govuk-table__cell" style="vertical-align: middle;">{{ subscription.dateAdded | date('D MMMM YYYY') }}</td> | ||
| <td class="govuk-table__cell govuk-table__cell--numeric" style="vertical-align: middle;"> | ||
| <div class="govuk-checkboxes__item govuk-checkboxes--small"> | ||
| <input | ||
| class="govuk-checkboxes__input row-checkbox" | ||
| id="court-{{ subscription.subscriptionId }}" | ||
| name="subscriptions" | ||
| type="checkbox" | ||
| value="{{ subscription.subscriptionId }}" | ||
| aria-label="Select {{ subscription.courtOrTribunalName }}" | ||
| {% if subscription.subscriptionId in previouslySelected %}checked{% endif %}> | ||
| <label class="govuk-label govuk-checkboxes__label" for="court-{{ subscription.subscriptionId }}"> | ||
| <span class="govuk-visually-hidden">Select {{ subscription.courtOrTribunalName }}</span> | ||
| </label> | ||
| </div> | ||
| </td> | ||
| </tr> | ||
| {% endfor %} | ||
| </tbody> | ||
| </table> | ||
| {% endif %} | ||
| {% endset %} |
There was a problem hiding this comment.
Critical: Duplicate IDs in the DOM violate HTML standards.
The three tab panels (allTabHtml, caseTabHtml, courtTabHtml) are all rendered in the DOM simultaneously (hidden with CSS by govukTabs). This creates duplicate IDs:
select-all-caseappears in both allTabHtml (line 43) and caseTabHtml (line 146)select-all-courtappears in both allTabHtml (line 95) and courtTabHtml (line 199)- Row checkboxes
case-{{ subscription.subscriptionId }}appear in both allTabHtml and caseTabHtml - Row checkboxes
court-{{ subscription.subscriptionId }}appear in both allTabHtml and courtTabHtml
This causes:
- HTML validation errors
- Accessibility issues (screen readers may announce wrong elements)
- JavaScript issues (document.getElementById will return only the first match)
- The select-all JavaScript may not work correctly across tabs
Solution: Make IDs unique per tab panel by adding a prefix:
{% set allTabHtml %}
{% if hasCaseSubscriptions %}
...
<input
class="govuk-checkboxes__input select-all-checkbox"
- id="select-all-case"
+ id="all-select-all-case"
type="checkbox"
- data-table="case-subscriptions-table"
+ data-table="all-case-subscriptions-table"
aria-label="Select all case subscriptions">
- <label class="govuk-label govuk-checkboxes__label" for="select-all-case">
+ <label class="govuk-label govuk-checkboxes__label" for="all-select-all-case">Also update table IDs to be unique:
- <table class="govuk-table" id="case-subscriptions-table">
+ <table class="govuk-table" id="all-case-subscriptions-table">Apply similar prefixes to all IDs in caseTabHtml and courtTabHtml panels, and update row checkbox IDs accordingly.
Also applies to: 133-186, 188-237
🤖 Prompt for AI Agents
In libs/verified-pages/src/pages/bulk-unsubscribe/index.njk around lines 29 to
131 (and also apply same pattern to the other panels at 133-186 and 188-237),
there are duplicate IDs across the three tab panels (allTabHtml, caseTabHtml,
courtTabHtml) causing validation, accessibility and JS issues; fix by
namespacing IDs per panel (e.g. prefix IDs with all-, case- or court-), make
table IDs unique (e.g. all-case-subscriptions-table / case-subscriptions-table /
court-subscriptions-table), update the select-all input IDs and their label for=
attributes (e.g. id="all-select-all-case" and for="all-select-all-case"), update
each row checkbox id and its label for= to include the panel prefix (e.g.
id="all-case-{{subscription.subscriptionId}}"), and ensure any JS that queries
these IDs or table data attributes is updated to use the new prefixed
IDs/selectors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/subscriptions/src/repository/service.ts (1)
152-164: Address the deduplication and partial-deletion concerns raised in previous review.The concerns raised in the previous review remain valid and unaddressed:
Duplicate IDs cause false authorization errors: If
subscriptionIdscontains duplicates, the count check will fail even though the user owns all subscriptions, resulting in a misleading "Unauthorized" error.Partial deletions are committed before validation: The deletion executes within a transaction at the query layer, but the count validation happens afterward in the service layer. If the user owns only a subset of the subscriptions, those owned subscriptions are deleted and committed before the "Unauthorized" error is thrown, leaving the database in an inconsistent state.
As recommended in the previous review, deduplicate the IDs and consider adding a pre-check to validate ownership before performing any deletions:
export async function deleteSubscriptionsByIds(subscriptionIds: string[], userId: string) { if (subscriptionIds.length === 0) { throw new Error("No subscriptions provided for deletion"); } + const uniqueIds = [...new Set(subscriptionIds)]; + + // Pre-check ownership to prevent partial deletions + const owned = await findSubscriptionsWithLocationByIds(uniqueIds, userId); + if (owned.length !== uniqueIds.length) { + throw new Error("Unauthorized: User does not own all selected subscriptions"); + } - const count = await deleteSubscriptionsByIdsQuery(subscriptionIds, userId); + const count = await deleteSubscriptionsByIdsQuery(uniqueIds, userId); - if (count !== subscriptionIds.length) { - throw new Error("Unauthorized: User does not own all selected subscriptions"); - } return count; }
🧹 Nitpick comments (2)
libs/subscriptions/src/repository/service.ts (2)
16-27: Consider adding a default value for thelocaleparameter.The
localeparameter has no default value here, though all callers pass"en"as the default. For consistency and to prevent potential issues if this internal helper is called from new locations, consider adding a default:-function mapSubscriptionToDto( - sub: { subscriptionId: string; locationId: number; dateAdded: Date; location: { name: string; welshName: string | null } }, - locale: string -): SubscriptionDto { +function mapSubscriptionToDto( + sub: { subscriptionId: string; locationId: number; dateAdded: Date; location: { name: string; welshName: string | null } }, + locale = "en" +): SubscriptionDto {
29-35: Move type definitions to the bottom of the file.Per coding guidelines, "interfaces and types at the bottom." The
SubscriptionDtointerface should be moved to the end of the file after all function definitions to follow the prescribed module ordering: constants, exported functions, other functions, then types/interfaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/subscriptions/src/repository/service.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/subscriptions/src/repository/service.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/subscriptions/src/repository/service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/subscriptions/src/repository/service.ts
🧬 Code graph analysis (1)
libs/subscriptions/src/repository/service.ts (1)
libs/subscriptions/src/repository/queries.ts (5)
findSubscriptionById(42-46)deleteSubscriptionRecord(48-53)findSubscriptionsWithLocationByUserId(55-63)findSubscriptionsWithLocationByIds(65-76)deleteSubscriptionsByIds(78-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (6)
libs/subscriptions/src/repository/service.ts (6)
6-11: LGTM!The import statements follow the coding guidelines correctly, using
.jsextensions for relative imports and descriptive names for the imported functions.
57-59: LGTM!The function properly delegates to the repository query layer with appropriate user scoping.
61-75: LGTM!The function uses a defensive pattern with both pre-check and post-check validation. While the double validation might seem redundant, it properly guards against race conditions where a subscription could be deleted between the existence check and the actual deletion.
118-118: LGTM!The deletion now properly includes the
userIdparameter for ownership validation, making the operation safer.
128-141: LGTM!The three functions provide a clean API surface for retrieving subscriptions:
getAllSubscriptionsByUserIdfetches and maps all subscriptions with proper locale handlinggetCaseSubscriptionsByUserIdis appropriately stubbed with a clear comment explaining the future implementationgetCourtSubscriptionsByUserIdprovides semantic clarity as a court-specific wrapperThe unused
localeparameter ingetCaseSubscriptionsByUserIdis acceptable given it's a placeholder for VIBE-300.
143-150: LGTM!The function handles the empty array case efficiently with an early return and properly delegates to the query layer for ownership-scoped retrieval before mapping to DTOs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/verified-pages/src/assets/js/select_all.ts (1)
16-31: Optional: reduce nested callbacks to address “nesting” warnings and improve readability.Example direction (not exhaustive): extract
onSelectAllChange/onRowChangehandler functions insidesetupSelectAllCheckboxand reuse them for listener registration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/verified-pages/src/assets/js/select_all.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/index.ts(1 hunks)libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/verified-pages/src/pages/confirm-bulk-unsubscribe/index.ts
- libs/verified-pages/src/pages/bulk-unsubscribe/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/verified-pages/src/assets/js/select_all.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/verified-pages/src/assets/js/select_all.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/verified-pages/src/assets/js/select_all.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
| }; | ||
| } | ||
|
|
||
| interface SubscriptionDto { |
There was a problem hiding this comment.
Can we move the interface definition to be above the function?
| bulkUnsubscribeTitle: "Bulk unsubscribe", | ||
| bulkUnsubscribeHeading: "Bulk unsubscribe", | ||
|
|
||
| // Tabs |
There was a problem hiding this comment.
Not sure if we really need comments in language resource files. They are not in language files in other pages.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/subscriptions/src/repository/service.ts (1)
152-164: Bulk delete semantics: deduplicate IDs and pre-check ownership to avoid partial successThe current
deleteSubscriptionsByIdsimplementation validates non‑empty input and then compares the delete count against the rawsubscriptionIds.length. This has two subtle edge cases:
- Duplicates in
subscriptionIds: If an ID appears multiple times, the underlying delete will remove that row once, socount < subscriptionIds.lengthand you throw"Unauthorized", even though all of the user’s subscriptions that matched were legitimately deleted.- Partial success before error: If the caller passes a mix of IDs the user owns and IDs they do not, all owned rows are deleted first, then the count mismatch triggers the
"Unauthorized"error. From a caller/UX perspective this looks like a failure, but some subscriptions were actually removed.To make this safer and more predictable, you could:
- Deduplicate IDs before any DB work.
- Pre‑validate ownership (e.g., via
findSubscriptionsWithLocationByIds) against those unique IDs and only proceed with deletion if the counts match, ensuring all‑or‑nothing behaviour.Roughly:
export async function deleteSubscriptionsByIds(subscriptionIds: string[], userId: string) { if (subscriptionIds.length === 0) { throw new Error("No subscriptions provided for deletion"); } const uniqueIds = [...new Set(subscriptionIds)]; // Optional pre-check to avoid partial deletes // const owned = await findSubscriptionsWithLocationByIds(uniqueIds, userId); // if (owned.length !== uniqueIds.length) { // throw new Error("Unauthorized: User does not own all selected subscriptions"); // } const count = await deleteSubscriptionsByIdsQuery(uniqueIds, userId); if (count !== uniqueIds.length) { throw new Error("Unauthorized: User does not own all selected subscriptions"); } return count; }This keeps the ownership guarantee while eliminating duplicate‑ID surprises and “partial success then error” behaviour.
🧹 Nitpick comments (3)
libs/subscriptions/src/repository/service.ts (3)
16-35: DTO and mapper look sound; consider type/placement tidy-upThe
SubscriptionDtoshape andmapSubscriptionToDtologic (including Welsh fallback whenlocale === "cy") match the query shapes fromfindSubscriptionsWithLocationByUserId/findSubscriptionsWithLocationByIds, and using"court"as thetypeis consistent with the current court-only implementation.Two minor follow‑ups you might consider:
- If case subscriptions will reuse this DTO, you may later want a dedicated mapper that sets
type: "case"to avoid overloading this one.- To align with the stated module ordering (constants → exported functions → helpers → types at bottom), moving
SubscriptionDto(and possiblymapSubscriptionToDto) nearer the bottom of the file would better match the guidelines, though this is purely structural. As per coding guidelines, ...
128-141: Court/case subscription getters are a reasonable first cut
getAllSubscriptionsByUserIdcorrectly reusesfindSubscriptionsWithLocationByUserIdand the shared DTO mapper, giving you a single source for locale‑aware naming.getCourtSubscriptionsByUserIddelegating togetAllSubscriptionsByUserIdmatches the current “court‑only” reality and keeps the public API stable for when case subscriptions land.getCaseSubscriptionsByUserIdreturning[]with clear comments about VIBE‑300 makes the current limitation explicit; just ensure callers are aware that this is intentionally a stub until that work is done.No changes required now; these are clean extension points for the upcoming case‑subscription work.
143-150: Confirmation details helper aligns with DTO mapping, but consider deduping IDs
getSubscriptionDetailsForConfirmationcorrectly:
- Short‑circuits on an empty
subscriptionIdsarray.- Scopes lookups by both
subscriptionIdsanduserId.- Reuses
mapSubscriptionToDtoto keep confirmation and listing views consistent.As a small robustness/perf tweak, you could deduplicate
subscriptionIdsbefore passing them tofindSubscriptionsWithLocationByIdsto keep theINlist minimal and the mapping logic simpler if upstream ever sends duplicates. This is optional given typical list sizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/subscriptions/src/repository/service.ts(4 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/cy.ts(1 hunks)libs/verified-pages/src/pages/bulk-unsubscribe/en.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/verified-pages/src/pages/bulk-unsubscribe/cy.ts
- libs/verified-pages/src/pages/bulk-unsubscribe/en.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
libs/subscriptions/src/repository/service.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
libs/subscriptions/src/repository/service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
libs/subscriptions/src/repository/service.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
libs/subscriptions/src/repository/service.ts
🧬 Code graph analysis (1)
libs/subscriptions/src/repository/service.ts (1)
libs/subscriptions/src/repository/queries.ts (5)
findSubscriptionById(42-46)deleteSubscriptionRecord(48-53)findSubscriptionsWithLocationByUserId(55-63)findSubscriptionsWithLocationByIds(65-76)deleteSubscriptionsByIds(78-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Changed Packages
- GitHub Check: E2E Tests
🔇 Additional comments (3)
libs/subscriptions/src/repository/service.ts (3)
6-11: New query imports are consistent and ESM-compliantThe additional imports from
./queries.jsline up with the new service functions and keep everything using ESM with explicit.jsextensions; no issues here.
57-75: User-scoped get/remove subscription logic is correct
getSubscriptionByIdand the updatedremoveSubscriptioncorrectly scope lookups and deletes byuserId, preventing cross‑user deletion. The double check inremoveSubscription(findSubscriptionById+deleteSubscriptionRecordwithuserId) safely handles races where the row disappears between read and delete by surfacing"Subscription not found"in both cases.
116-120: Updated delete call in replaceUserSubscriptions is consistent with user scopingUsing
deleteSubscriptionRecord(sub.subscriptionId, userId)keeps deletions aligned with the user‑scoped query helpers inqueries.tsand avoids any future accidental cross‑user deletes if the function is reused. The surrounding validation and max‑limit logic remain unchanged.
|
4 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-306
Change description
Added bulk unsubscribe flow for verified users. Case subscription functionality has been created, but will not display at present until VIBE-300 has been played.
Summary by CodeRabbit
New Features
Localization
UI
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.