VIBE-192 Add and Remove Subscription for Verified Users #130
Conversation
Created comprehensive specification and task breakdown for implementing email subscription functionality for verified users in CaTH. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Generate specification and technical implementation plan for verified user email subscription functionality. Includes four-page flow with venue selection, confirmation logic, and duplicate prevention. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Specification document with user flow and requirements - Technical implementation plan with module architecture - Detailed task breakdown with 27 actionable tasks - Estimated effort: 4-5 days (31-40 hours) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Created comprehensive technical documentation for verified user email subscriptions: - specification.md: Full requirements and acceptance criteria - plan.md: Detailed technical architecture and implementation guide - tasks.md: Phased implementation tasks with estimates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Created comprehensive documentation for the email subscriptions feature following HMCTS monorepo standards and GOV.UK Design System patterns. Specification includes: - 4-page user flow (dashboard, add, confirm, confirmation) - Database schema with subscription table - Welsh language support requirements - WCAG 2.2 AA accessibility compliance - Validation rules and error handling - Test scenarios (unit, integration, E2E) Technical plan includes: - Step-by-step implementation guide - Complete code examples for all pages - Database queries and business logic - GOV.UK component usage - Module registration steps - Security and performance requirements 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
WalkthroughAdds a Verified User Email Subscriptions feature: new subscriptions library, Prisma schema and migrations, service/validation layers, UI pages (manage, search, pending, confirm, delete, unsubscribe), localization (EN/CY), navigation changes, comprehensive unit and E2E tests, and build/config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant WebApp as Web App (Express handlers)
participant Session
participant LocationSvc as `@hmcts/location`
participant SubService as `@hmcts/subscriptions` (service)
participant Postgres
User->>Browser: Click "Add subscription"
Browser->>WebApp: GET /location-name-search
WebApp->>LocationSvc: fetch jurisdictions/locations
LocationSvc-->>WebApp: locations data
WebApp-->>Browser: Render search page
User->>Browser: Select locations, Submit
Browser->>WebApp: POST /location-name-search (locationIds)
WebApp->>Session: store pendingSubscriptions
Session-->>WebApp: saved
WebApp-->>Browser: redirect /pending-subscriptions
Browser->>WebApp: GET /pending-subscriptions
WebApp->>Session: read pendingSubscriptions
WebApp-->>Browser: render pending list
User->>Browser: Confirm
Browser->>WebApp: POST /pending-subscriptions (action=confirm)
WebApp->>SubService: replaceUserSubscriptions(userId, mergedIds)
SubService->>Postgres: read/create/delete subscription rows
Postgres-->>SubService: result
SubService-->>WebApp: success
WebApp->>Session: clear pending, set confirmationComplete
WebApp-->>Browser: redirect /subscription-confirmed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 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)
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: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
libs/email-subscriptions/src/pages/pending-subscriptions/en.ts-7-7 (1)
7-7: Fix inconsistent capitalization in user-facing text.The text "Add another email Subscription" has inconsistent capitalization. The word "subscription" should be lowercase to match standard English conventions and maintain consistency with other UI strings in the application.
Apply this diff:
- addAnotherSubscription: "Add another email Subscription", + addAnotherSubscription: "Add another email subscription",libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts-7-7 (1)
7-7: Fix inconsistent Welsh translation punctuation.The Welsh translation for
inOrderTois "er mwyn:" (with colon), but inlibs/email-subscriptions/src/pages/subscription-confirmed/cy.tsat line 6, the same key uses "er mwyn" (without colon). This inconsistency should be resolved for a uniform user experience.Apply this diff to align with the other usage:
- inOrderTo: "er mwyn:", + inOrderTo: "er mwyn",libs/email-subscriptions/src/subscription/validation.ts-4-7 (1)
4-7: Remove unnecessaryasynckeyword fromvalidateLocationId.The function
getLocationByIdis synchronous (defined inlibs/location/src/repository/queries.ts:16asexport function getLocationById(id: number): Location | undefined). SincevalidateLocationIddoesn't perform any async operations and doesn't awaitgetLocationById, remove theasynckeyword and change the return type toboolean:-export async function validateLocationId(locationId: string): Promise<boolean> { +export function validateLocationId(locationId: string): boolean { const location = getLocationById(Number.parseInt(locationId, 10)); return location !== undefined; }libs/email-subscriptions/package.json-1-27 (1)
1-27: Declare workspace dependencies explicitlyThe package imports
@hmcts/auth,@hmcts/location, and@hmcts/postgresthroughout its source code but does not declare them inpackage.json. Add these as"dependencies"with"workspace:*"versions to make the dependency graph explicit:"dependencies": { "@hmcts/auth": "workspace:*", "@hmcts/location": "workspace:*", "@hmcts/postgres": "workspace:*" }This ensures tooling and future refactors correctly identify the real dependency structure.
(Note: Express
^5.1.0is already aligned across the monorepo, so no change is needed there.)
🧹 Nitpick comments (19)
libs/email-subscriptions/src/pages/unsubscribe-confirmation/en.ts (1)
7-7: Minor punctuation inconsistency.Line 7 includes a colon after "in order to:", but the similar file
libs/email-subscriptions/src/pages/subscription-confirmed/en.tsuses "in order to" without a colon. Consider aligning the punctuation for consistency across the localization files.Apply this diff for consistency:
- inOrderTo: "in order to:", + inOrderTo: "in order to",docs/tickets/VIBE-192/plan.md (1)
11-11: Add language specification to fenced code block.The fenced code block should specify a language for proper syntax highlighting and markdown compliance.
Based on static analysis tools.
Apply this diff:
-``` +```text libs/email-subscriptions/VIBE-192-plan.md (2)
16-20: Update module path in the plan to match the implemented locationThe plan still refers to
libs/subscriptions/, but the implementation lives underlibs/email-subscriptions/. Consider updating these references (module, file paths, route examples) so the plan accurately reflects the current structure.- - **Module**: Subscription management module at `libs/subscriptions/` + - **Module**: Subscription management module at `libs/email-subscriptions/`(And similarly for other
libs/subscriptions/paths in this document.)
640-651: Optional: Fix markdownlint MD036 warning on the total effort lineThe final
**Total: ~25 hours**line is flagged as “emphasis used instead of a heading” (MD036). If you want markdownlint clean, you could convert this to a heading or plain text.-**Total: ~25 hours** +### Total effort + +~25 hoursapps/postgres/src/schema-discovery.test.ts (1)
15-19: Test is correct but tightly coupled to there being exactly one schemaCurrent expectations are valid, but
expect(result.length).toBe(1);will need editing as soon as additional Prisma schemas are added. You could instead just assert that at least one entry contains"email-subscriptions"to make this test more future-proof.- const result = getPrismaSchemas(); - expect(result.length).toBe(1); - expect(result[0]).toContain("email-subscriptions"); + const result = getPrismaSchemas(); + expect(result).toEqual( + expect.arrayContaining([expect.stringContaining("email-subscriptions")]) + );libs/email-subscriptions/src/subscription/validation.test.ts (1)
1-69: Validator test coverage is solid; only minor optional polish possibleThe tests cover the key paths for both validators (valid/invalid/non‑numeric IDs and duplicate/non‑duplicate subscriptions) and exercise the external dependencies via mocks appropriately. As an optional tidy‑up, you could:
- Add an assertion on
findSubscriptionByUserAndLocationin the “should return false if subscription exists” test for symmetry.- Consider resetting mocks in a
beforeEachif you later expand this suite.libs/email-subscriptions/src/pages/subscription-confirmed/en.ts (1)
1-11: EN copy looks consistent; consider adding a colon after “in order to”The localisation structure and keys align with the other subscription pages and should integrate cleanly. If you want the sentence before the bullet list to mirror the unsubscribe‑confirmation pattern, you could change
inOrderToto"in order to:", but that’s purely a content/design choice.libs/email-subscriptions/src/pages/delete-subscription/index.njk (1)
13-14: Use<h1>for the main page headingThis appears to be the primary page heading, so it should normally be rendered as an
<h1 class="govuk-heading-l">for consistency with other pages (e.g. pending‑subscriptions) and GOV.UK accessibility guidance.libs/email-subscriptions/src/pages/pending-subscriptions/index.njk (1)
63-68: Localise table headers and avoid reusing the page heading as a column labelIn the subscriptions table, the first header cell reuses
{{ heading }}and the second is hard‑coded to"Actions". For clarity and localisation:
- Introduce a dedicated key for the first column (e.g.
tableHeaderLocation) instead of repeating the page heading.- Move
"Actions"into the locale files (EN/CY) so the Welsh version doesn’t display an English header.Also applies to: 88-90, 94-96
libs/email-subscriptions/src/pages/subscription-management/index.njk (1)
22-22: Remove aria-sort attribute if sorting is not implemented.The table header includes
aria-sort="none", but there's no evidence of sorting functionality in the template. This could confuse screen reader users who expect sortable columns.- <th scope="col" class="govuk-table__header govuk-!-width-three-quarters" aria-sort="none">{{ tableHeaderLocation }}</th> + <th scope="col" class="govuk-table__header govuk-!-width-three-quarters">{{ tableHeaderLocation }}</th>libs/email-subscriptions/src/subscription/validation.ts (2)
5-5: Handle invalid locationId parsing explicitly.
Number.parseIntreturnsNaNfor invalid input, which is then passed togetLocationById. While this likely works as intended (returning undefined for invalid IDs), it would be clearer to handle this case explicitly.export async function validateLocationId(locationId: string): Promise<boolean> { - const location = getLocationById(Number.parseInt(locationId, 10)); + const numericId = Number.parseInt(locationId, 10); + if (Number.isNaN(numericId)) { + return false; + } + const location = getLocationById(numericId); return location !== undefined; }
9-12: Consider renaming for clarity.The function name
validateDuplicateSubscriptioncould be confusing since it returnstruewhen there is NO duplicate (i.e., the subscription is valid to create). Consider a name that more clearly indicates it's checking availability.-export async function validateDuplicateSubscription(userId: string, locationId: string): Promise<boolean> { +export async function isSubscriptionAvailable(userId: string, locationId: string): Promise<boolean> { const existing = await findSubscriptionByUserAndLocation(userId, locationId); return !existing; }Or keep the current name but add a JSDoc comment:
+/** + * Validates that a subscription doesn't already exist for the given user and location. + * @returns true if the subscription can be created (no duplicate exists), false otherwise + */ export async function validateDuplicateSubscription(userId: string, locationId: string): Promise<boolean> {libs/email-subscriptions/src/pages/subscription-management/index.ts (1)
16-22: Handle invalid locationId parsing explicitly.
Number.parseIntcan returnNaNfor invalid strings, which is then passed togetLocationById. While the fallback tosub.locationIdhandles this, it would be clearer to check for NaN explicitly.const subscriptionsWithDetails = subscriptions.map((sub) => { - const location = getLocationById(Number.parseInt(sub.locationId, 10)); + const numericId = Number.parseInt(sub.locationId, 10); + const location = Number.isNaN(numericId) ? undefined : getLocationById(numericId); return { ...sub, locationName: location ? (locale === "cy" ? location.welshName : location.name) : sub.locationId }; });libs/email-subscriptions/src/pages/location-name-search/index.njk (1)
66-111: Tidy up accessibility details for toggles and back‑to‑top linkThe template looks functionally sound, but a couple of a11y tweaks would help:
- The filter section toggle buttons have
aria-expanded="true"hard‑coded and the icon always shows “−”. Ensure whatever JS handles collapsing/expanding also updatesaria-expandedand the icon so screen readers get the correct state.- The “Back to top” link uses
href="#". Prefer targeting a specific anchor (for example,href="#top"with an elementid="top"near the page start) so focus and scroll behaviour are predictable and don’t depend on browser quirks.These are non‑blocking, but worth aligning with GOV.UK a11y guidance.
Also applies to: 124-181, 184-188
docs/tickets/VIBE-192/tasks.md (1)
1-424: Tasks doc is solid; only minor markdown style nits if you careThe implementation checklist here is thorough and matches the current
libs/email-subscriptionsnaming and design. From a code/architecture perspective, nothing blocking.If you want to keep markdown linters fully happy, you could:
- Turn the emphasized “Total: 12–17 days (2.5–3.5 weeks)” line into a proper heading (
### Total effort) instead of bold text.- Optionally hyphenate compound adjectives like “rate‑limiting” when used before a noun (e.g. “rate‑limiting configuration”) to satisfy stricter style tools.
Purely cosmetic; no functional impact.
.ai/plans/VIBE-192/plan.md (1)
16-22: Plan document is partially stale vs. the implementedemail-subscriptionsmoduleThis technical plan still talks about a
libs/subscriptionsmodule and@hmcts/subscriptionsalias with/subscriptions/...routes, while the implementation in this PR has landed underlibs/email-subscriptionswith@hmcts/email-subscriptionsand a slightly different page structure.To avoid confusion for anyone using this as a blueprint later, I’d either:
- Update the module paths, aliases, and example route URLs to match the concrete implementation, or
- Add a short disclaimer at the top that this describes an earlier prototype design and link to the up‑to‑date docs under
docs/tickets/VIBE-192/.The content is still useful, but clarifying its relationship to the current codebase will help future maintainers.
Also applies to: 63-90, 107-166, 929-1050
.ai/plans/VIBE-192/specification.md (1)
22-104: Specification is slightly out of sync with the shipped email‑subscriptions implementationThis spec still assumes:
- Routes under
/subscriptions/...(list, add, confirm, success, remove).- A
Subscriptiontable withlocationId Int.The implementation in this PR for verified users lives under
libs/email-subscriptions, uses different route names for the management pages, and the Prisma model defineslocationIdasString. That’s fine if this doc is intentionally generic or describes a previous iteration, but it may be confusing when cross‑referenced with the current code.Consider either:
- Updating the table definition and URL examples here to match the actual schema and routes, or
- Adding a brief note near the top clarifying that this spec is conceptual and that the concrete implementation is documented under
docs/tickets/VIBE-192/.No functional impact today, just a documentation alignment point.
Also applies to: 107-122, 194-212
docs/tickets/VIBE-192/specification.md (1)
63-101: Add languages to fenced code blocks to satisfy markdownlint and aid toolingSeveral fenced blocks (for the “Empty state”, “With Subscriptions”, search/browse layouts, and confirm/confirmation layouts) use bare triple backticks without a language. This triggers the MD040 warnings and also makes syntax highlighting less useful.
Given these are mostly pseudo‑UI snippets, you could use a generic language like
text(ormdwhere appropriate), e.g.:```text Your email subscriptions ──────────────────────── ...Apply a language tag consistently to the affected blocks throughout the spec. Also applies to: 122-178, 199-305 </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/service.ts (1)</summary><blockquote> `52-63`: **Consider transactional replace and deduping new IDs for robustness** `createMultipleSubscriptions` and `replaceUserSubscriptions` are doing multi‑row operations without a transaction: - In `replaceUserSubscriptions`, deletes and creates are issued via `Promise.all`. If one operation fails (e.g. DB error, unique constraint), you can end up with a partially updated subscription set. - `replaceUserSubscriptions` uses raw `newLocationIds` to build `toAdd`; if the array contains duplicates that aren’t in the existing set, you’ll attempt multiple creates for the same `(userId, locationId)` and rely on DB constraints to fail. Consider: - Wrapping the `toDelete`/`toAdd` operations in a `prisma.$transaction([...])` to make the replacement atomic. - Normalising `newLocationIds` with `Array.from(new Set(newLocationIds))` before computing `toAdd` to avoid duplicate inserts. Not strictly required for correctness in simple flows, but will make the service more resilient under edge cases and concurrency. Also applies to: 65-81 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2e46da71f91f36665cc0db5ac70ec7582a23db04 and cb63eae875c073ddde63dd85b8d4864993b36294. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `yarn.lock` is excluded by `!**/yarn.lock`, `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (53)</summary> * `.ai/plans/VIBE-192/plan.md` (1 hunks) * `.ai/plans/VIBE-192/specification.md` (1 hunks) * `.ai/plans/VIBE-192/tasks.md` (1 hunks) * `VIBE-192-plan.md` (1 hunks) * `VIBE-192-specification.md` (1 hunks) * `apps/postgres/prisma/migrations/20251124104824_add_email_subscriptions/migration.sql` (1 hunks) * `apps/postgres/prisma/migrations/20251124165145_subscription_table/migration.sql` (1 hunks) * `apps/postgres/src/schema-discovery.test.ts` (1 hunks) * `apps/postgres/src/schema-discovery.ts` (1 hunks) * `apps/web/src/app.test.ts` (1 hunks) * `apps/web/src/app.ts` (3 hunks) * `docs/tickets/VIBE-192/plan.md` (1 hunks) * `docs/tickets/VIBE-192/specification.md` (1 hunks) * `docs/tickets/VIBE-192/tasks.md` (1 hunks) * `libs/email-subscriptions/package.json` (1 hunks) * `libs/email-subscriptions/prisma/schema.prisma` (1 hunks) * `libs/email-subscriptions/src/config.ts` (1 hunks) * `libs/email-subscriptions/src/index.ts` (1 hunks) * `libs/email-subscriptions/src/locales/cy.ts` (1 hunks) * `libs/email-subscriptions/src/locales/en.ts` (1 hunks) * `libs/email-subscriptions/src/pages/delete-subscription/cy.ts` (1 hunks) * `libs/email-subscriptions/src/pages/delete-subscription/en.ts` (1 hunks) * `libs/email-subscriptions/src/pages/delete-subscription/index.njk` (1 hunks) * `libs/email-subscriptions/src/pages/delete-subscription/index.ts` (1 hunks) * `libs/email-subscriptions/src/pages/location-name-search/cy.ts` (1 hunks) * `libs/email-subscriptions/src/pages/location-name-search/en.ts` (1 hunks) * `libs/email-subscriptions/src/pages/location-name-search/index.njk` (1 hunks) * `libs/email-subscriptions/src/pages/location-name-search/index.ts` (1 hunks) * `libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts` (1 hunks) * `libs/email-subscriptions/src/pages/pending-subscriptions/en.ts` (1 hunks) * `libs/email-subscriptions/src/pages/pending-subscriptions/index.njk` (1 hunks) * `libs/email-subscriptions/src/pages/pending-subscriptions/index.ts` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-confirmed/en.ts` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-confirmed/index.njk` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-confirmed/index.ts` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-management/cy.ts` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-management/en.ts` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-management/index.njk` (1 hunks) * `libs/email-subscriptions/src/pages/subscription-management/index.ts` (1 hunks) * `libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts` (1 hunks) * `libs/email-subscriptions/src/pages/unsubscribe-confirmation/en.ts` (1 hunks) * `libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.njk` (1 hunks) * `libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts` (1 hunks) * `libs/email-subscriptions/src/session-types.ts` (1 hunks) * `libs/email-subscriptions/src/subscription/queries.test.ts` (1 hunks) * `libs/email-subscriptions/src/subscription/queries.ts` (1 hunks) * `libs/email-subscriptions/src/subscription/service.test.ts` (1 hunks) * `libs/email-subscriptions/src/subscription/service.ts` (1 hunks) * `libs/email-subscriptions/src/subscription/validation.test.ts` (1 hunks) * `libs/email-subscriptions/src/subscription/validation.ts` (1 hunks) * `libs/email-subscriptions/tsconfig.json` (1 hunks) * `tsconfig.json` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (2)</summary> <details> <summary>📚 Learning: 2025-11-20T09:59:16.753Z</summary>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.753Z
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/email-subscriptions/src/pages/subscription-confirmed/cy.ts` - `libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts` - `libs/email-subscriptions/src/locales/cy.ts` - `libs/email-subscriptions/src/pages/location-name-search/cy.ts` - `libs/email-subscriptions/src/pages/delete-subscription/cy.ts` - `libs/email-subscriptions/src/pages/subscription-management/cy.ts` </details> <details> <summary>📚 Learning: 2025-11-20T10:19:35.864Z</summary>Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 106
File: libs/system-admin-pages/src/reference-data-upload/services/download-service.ts:25-27
Timestamp: 2025-11-20T10:19:35.864Z
Learning: In the HMCTS cath-service project, region and sub-jurisdiction names are managed manually and must not contain semicolons, as semicolons are used as delimiters when joining multiple values in CSV export/import operations (specifically in libs/system-admin-pages/src/reference-data-upload/services/download-service.ts).**Applied to files:** - `tsconfig.json` </details> </details><details> <summary>🧬 Code graph analysis (20)</summary> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)</summary> * `cy` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)</summary> * `cy` (1-23) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (1)</summary> * `cy` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary> * `cy` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (1)</summary> * `cy` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/en.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/en.ts (1)</summary> * `en` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/en.ts (1)</summary> * `en` (1-23) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/en.ts (1)</summary> * `en` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/en.ts (1)</summary> * `en` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/en.ts (1)</summary> * `en` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/en.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/en.ts (1)</summary> * `en` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/en.ts (1)</summary> * `en` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/en.ts (1)</summary> * `en` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/en.ts (1)</summary> * `en` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/en.ts (1)</summary> * `en` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)</summary> * `cy` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)</summary> * `cy` (1-23) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)</summary> * `cy` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary> * `cy` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (1)</summary> * `cy` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/validation.test.ts (1)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/subscription/validation.ts (2)</summary> * `validateLocationId` (4-7) * `validateDuplicateSubscription` (9-12) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/validation.ts (1)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/subscription/queries.ts (1)</summary> * `findSubscriptionByUserAndLocation` (14-23) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/cy.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)</summary> * `cy` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (1)</summary> * `cy` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)</summary> * `cy` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary> * `cy` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (1)</summary> * `cy` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/service.test.ts (1)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/subscription/service.ts (5)</summary> * `createSubscription` (13-30) * `getSubscriptionsByUserId` (32-34) * `removeSubscription` (36-50) * `createMultipleSubscriptions` (52-63) * `replaceUserSubscriptions` (65-97) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)</summary> * `cy` (1-23) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (1)</summary> * `cy` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)</summary> * `cy` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary> * `cy` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (1)</summary> * `cy` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/en.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/en.ts (1)</summary> * `en` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/en.ts (1)</summary> * `en` (1-23) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/en.ts (1)</summary> * `en` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/en.ts (1)</summary> * `en` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/en.ts (1)</summary> * `en` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/index.ts (3)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary> * `cy` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/en.ts (1)</summary> * `en` (1-10) </details> <details> <summary>libs/email-subscriptions/src/subscription/service.ts (1)</summary> * `getSubscriptionsByUserId` (32-34) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/service.ts (2)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/subscription/validation.ts (1)</summary> * `validateLocationId` (4-7) </details> <details> <summary>libs/email-subscriptions/src/subscription/queries.ts (4)</summary> * `findSubscriptionByUserAndLocation` (14-23) * `countSubscriptionsByUserId` (25-31) * `createSubscriptionRecord` (33-40) * `deleteSubscriptionRecord` (42-46) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)</summary> * `cy` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)</summary> * `cy` (1-23) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (1)</summary> * `cy` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)</summary> * `cy` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary> * `cy` (1-10) </details> </blockquote></details> <details> <summary>apps/postgres/src/schema-discovery.test.ts (1)</summary><blockquote> <details> <summary>apps/postgres/src/schema-discovery.ts (1)</summary> * `getPrismaSchemas` (4-6) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/index.ts (2)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)</summary> * `cy` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/en.ts (1)</summary> * `en` (1-7) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/index.ts (2)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)</summary> * `cy` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/en.ts (1)</summary> * `en` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (5)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)</summary> * `cy` (1-7) </details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)</summary> * `cy` (1-23) </details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (1)</summary> * `cy` (1-12) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)</summary> * `cy` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (1)</summary> * `cy` (1-11) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/queries.test.ts (1)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/subscription/queries.ts (5)</summary> * `findSubscriptionsByUserId` (3-12) * `findSubscriptionByUserAndLocation` (14-23) * `countSubscriptionsByUserId` (25-31) * `createSubscriptionRecord` (33-40) * `deleteSubscriptionRecord` (42-46) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/en.ts (2)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/en.ts (1)</summary> * `en` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/en.ts (1)</summary> * `en` (1-10) </details> </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts (6)</summary><blockquote> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary> * `cy` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (1)</summary> * `cy` (1-11) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/en.ts (1)</summary> * `en` (1-10) </details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/en.ts (1)</summary> * `en` (1-11) </details> <details> <summary>libs/email-subscriptions/src/subscription/service.ts (1)</summary> * `removeSubscription` (36-50) </details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/index.ts (1)</summary> * `GET` (37-37) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/tickets/VIBE-192/tasks.md</summary> [style] ~315-~315: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym. Context: ... associated - [ ] Verify error messages properly linked - [ ] Test with 200% text zoom ... (ADVERB_REPETITION_PREMIUM) --- [uncategorized] ~330-~330: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...tokens expire after 7 days - [ ] Verify rate limiting works - [ ] Verify SQL injection preven... (EN_COMPOUND_ADJECTIVE_INTERNAL) --- [uncategorized] ~402-~402: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...y API account and templates - Redis for rate limiting and token storage - PostgreSQL database... (EN_COMPOUND_ADJECTIVE_INTERNAL) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>VIBE-192-plan.md</summary> 651-651: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> <details> <summary>docs/tickets/VIBE-192/tasks.md</summary> 394-394: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> <details> <summary>VIBE-192-specification.md</summary> 88-88: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 128-128: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 153-153: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 190-190: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 219-219: Tables should be surrounded by blank lines (MD058, blanks-around-tables) --- 285-285: Tables should be surrounded by blank lines (MD058, blanks-around-tables) </details> <details> <summary>docs/tickets/VIBE-192/specification.md</summary> 63-63: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 76-76: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 122-122: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 137-137: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 162-162: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 199-199: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 219-219: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 240-240: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 274-274: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 290-290: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>.ai/plans/VIBE-192/plan.md</summary> 109-109: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 151-151: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 1293-1293: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> <details> <summary>docs/tickets/VIBE-192/plan.md</summary> 11-11: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>.ai/plans/VIBE-192/tasks.md</summary> 23-23: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 33-33: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 40-40: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 53-53: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 59-59: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 67-67: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 82-82: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 92-92: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 101-101: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 111-111: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 126-126: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 134-134: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 151-151: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 164-164: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 179-179: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 188-188: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 198-198: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 209-209: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 217-217: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 223-223: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 241-241: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 254-254: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 269-269: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 282-282: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 289-289: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 302-302: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 310-310: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 319-319: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 337-337: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: SonarQube Analysis * GitHub Check: E2E Tests </details> <details> <summary>🔇 Additional comments (28)</summary><blockquote> <details> <summary>libs/email-subscriptions/tsconfig.json (1)</summary><blockquote> `1-11`: **LGTM!** The TypeScript configuration is well-structured for a library package, with appropriate declaration generation enabled for downstream consumers and sensible include/exclude patterns. </blockquote></details> <details> <summary>libs/email-subscriptions/src/locales/en.ts (1)</summary><blockquote> `1-7`: **LGTM!** The English locale strings are clear and appropriately defined for common UI actions. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)</summary><blockquote> `1-7`: **LGTM!** The Welsh translations are appropriately structured for the delete subscription confirmation flow. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (1)</summary><blockquote> `1-12`: **LGTM!** The Welsh translations are well-structured and mirror the English locale appropriately for the pending subscriptions page. </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/service.test.ts (1)</summary><blockquote> `1-301`: **Comprehensive test coverage with good edge case handling.** The test suite provides excellent coverage of the subscription service, including success paths, error conditions, authorization checks, and boundary conditions (e.g., max subscriptions). The test structure effectively uses mocks to isolate the service layer. </blockquote></details> <details> <summary>apps/postgres/prisma/migrations/20251124104824_add_email_subscriptions/migration.sql (1)</summary><blockquote> `1-23`: **No issues found - schema and migrations are properly aligned.** Verification confirms: 1. ✓ The Prisma schema correctly reflects the final state after both migrations (20251124104824 and 20251124165145) are applied 2. ✓ The migration sequence is intentional: the first migration establishes the table with initial fields, the second refactors it to drop `is_active`, `subscribed_at`, `unsubscribed_at` and add `date_added` 3. ✓ The `idx_subscription_active` index is properly handled—it's explicitly dropped in the subsequent migration (line 10 of 20251124165145_subscription_table), preventing any conflicts The current migration file is correct as written and aligns with the complete migration sequence and final schema state. </blockquote></details> <details> <summary>libs/email-subscriptions/src/locales/cy.ts (1)</summary><blockquote> `1-7`: **LGTM!** The Welsh localization strings are properly structured and follow the consistent pattern used across the email-subscriptions module. The translations appear accurate and appropriate for the common UI elements. </blockquote></details> <details> <summary>apps/postgres/src/schema-discovery.ts (1)</summary><blockquote> `2-5`: **LGTM!** The schema discovery integration correctly imports and returns the email-subscriptions Prisma schema path. This enables proper schema discovery for the new module, and the related test updates confirm the integration is working as expected. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/delete-subscription/en.ts (1)</summary><blockquote> `1-7`: **LGTM!** The English localization strings for the delete subscription confirmation follow standard GOV.UK Design System patterns for confirmation pages with Yes/No radio buttons. The wording is clear and appropriate. </blockquote></details> <details> <summary>apps/web/src/app.test.ts (2)</summary><blockquote> `136-137`: **LGTM!** The test expectations are correctly updated to account for the new email-subscriptions routes being registered. The comment clearly documents all 8 route registrations, including the newly added email-subscriptions pages. --- `144-145`: **LGTM!** The test assertion is appropriately updated to expect at least 8 calls to `createSimpleRouter`, accounting for the system-admin routes and the newly added email-subscriptions routes. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)</summary><blockquote> `1-10`: **Welsh translations are well-formed.** The Welsh localization follows the consistent pattern used across the email-subscriptions module, and the translations appear accurate. Apart from the "active subscriptions" terminology issue flagged separately, the localization structure is correct. </blockquote></details> <details> <summary>libs/email-subscriptions/src/index.ts (1)</summary><blockquote> `1-2`: **Barrel exports look correct and consistent** Re-exports of `subscription/service` and `subscription/validation` via `.js` paths are aligned with the nodenext ESM style used elsewhere; no issues spotted. </blockquote></details> <details> <summary>tsconfig.json (1)</summary><blockquote> `25-26`: **New path alias is consistent with existing module mappings** The `@hmcts/email-subscriptions` path points at the expected `libs/email-subscriptions/src` root and follows the same pattern as other HMCTS module aliases. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/index.njk (1)</summary><blockquote> `1-32`: **Subscription-confirmed template is well-structured and locale-ready** Template wiring (panel, body copy, and navigation links) lines up with the en/cy locale keys and existing GOV.UK layout patterns; no issues spotted here. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)</summary><blockquote> `1-11`: **Welsh translations align with the English structure** The `cy` locale mirrors the English key structure for the subscription-confirmed page, which keeps templates straightforward and consistent across languages. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/subscription-management/en.ts (1)</summary><blockquote> `1-10`: **Subscription-management English copy and keys look consistent** The English strings and key names match the Welsh counterpart and other pages’ patterns, so the subscription-management view will integrate cleanly with the locale selection logic. </blockquote></details> <details> <summary>apps/web/src/app.ts (1)</summary><blockquote> `8-8`: **Email-subscriptions module wiring into the web app looks correct** Importing `moduleRoot`/`pageRoutes`, adding `emailSubscriptionsModuleRoot` to `modulePaths`, and registering `emailSubscriptionsRoutes` with `createSimpleRouter(..., pageRoutes)` all mirror the existing patterns for other modules; the integration looks sound. Also applies to: 66-68, 105-106 </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)</summary><blockquote> `1-23`: **CY localisation structure matches EN and looks ready to use** The Welsh strings mirror the English keys (including `subJurisdictionLabels`), so this should integrate cleanly with the existing location‑name‑search page logic. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.njk (1)</summary><blockquote> `1-27`: **Unsubscribe confirmation template is clean and correctly wired to localisation** The panel, follow‑up sentence, and navigation links all consume the expected locale keys and point to the relevant routes (`/account-home`, `/location-name-search`, `/subscription-management`, `/search`). No changes needed from a templating perspective. </blockquote></details> <details> <summary>libs/email-subscriptions/src/session-types.ts (1)</summary><blockquote> `1-13`: **Session typing augmentation is appropriate; ensure it’s included by the TS project** The `express-session` module augmentation cleanly models the `emailSubscriptions` session bag and a generic `successMessage`, which should make downstream code much clearer. Just make sure this file is included in the relevant `tsconfig` (e.g. via `include` or being under a covered `rootDir`) so the augmentation is visible to the compiler wherever `SessionData` is used. </blockquote></details> <details> <summary>libs/email-subscriptions/src/pages/location-name-search/en.ts (1)</summary><blockquote> `1-23`: **EN localisation for location-name-search is consistent and complete** The exported `en` object provides all the keys needed by the page (including `subJurisdictionLabels`), and aligns with the CY counterpart and other subscription-related locale files. </blockquote></details> <details> <summary>.ai/plans/VIBE-192/tasks.md (1)</summary><blockquote> `1-436`: **LGTM - Comprehensive task breakdown.** The task breakdown is well-structured and provides clear guidance for implementing the email subscriptions feature. The phased approach with time estimates is helpful for project planning. </blockquote></details> <details> <summary>VIBE-192-specification.md (1)</summary><blockquote> `1-309`: **LGTM - Well-documented specification.** The specification provides comprehensive coverage of the feature requirements, including user journeys, page wireframes, data models, and test scenarios. The bilingual support (EN/CY) is properly documented. </blockquote></details> <details> <summary>libs/email-subscriptions/src/config.ts (1)</summary><blockquote> `1-9`: **LGTM - Clean module configuration.** The path resolution using ESM `import.meta.url` is correct, and the exported constants provide proper integration points for the web app and schema discovery. </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/queries.test.ts (1)</summary><blockquote> `1-186`: **LGTM - Comprehensive test coverage.** The test suite provides good coverage of all query functions with both happy paths and edge cases. The mocking strategy is appropriate for unit tests. </blockquote></details> <details> <summary>libs/email-subscriptions/prisma/schema.prisma (1)</summary><blockquote> `1-21`: **Subscription Prisma model looks consistent with the intended behaviour** The `Subscription` model mirrors the documented requirements: - UUID primary key (`subscription_id`) plus `user_id` and `location_id` fields. - Composite uniqueness on `(userId, locationId)` to prevent duplicates. - Indexes on `userId` and `locationId` to keep lookups fast. - `locationId` as `String` aligns with the “string, indexed” description in the VIBE‑192 tasks doc and works fine with form‑submitted IDs. I don’t see any schema issues based on the surrounding service code and docs. </blockquote></details> <details> <summary>libs/email-subscriptions/src/subscription/queries.ts (1)</summary><blockquote> `3-12`: **Review comment is based on incorrect assumptions and should be disregarded** The verification shows the code is correct: - **Timestamp field**: The schema uses `dateAdded` (not `subscribedAt`), and queries.ts correctly orders by `dateAdded`. The code is aligned with the actual schema. - **Delete strategy**: The schema has no `isActive` field and contains no soft-delete markers. `deleteSubscriptionRecord` correctly uses hard delete (`prisma.subscription.delete()`), which is the appropriate strategy for the current schema design. The review comment references a spec with `subscribedAt` and soft-delete semantics via `isActive`, but: - The actual Prisma schema does not contain an `isActive` field - No such specification or documentation exists in the email-subscriptions package - The implementation correctly matches the actual schema > Likely an incorrect or invalid review comment. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
apps/postgres/prisma/migrations/20251124165145_subscription_table/migration.sql
Outdated
Show resolved
Hide resolved
libs/email-subscriptions/src/pages/subscription-management/index.njk
Outdated
Show resolved
Hide resolved
libs/email-subscriptions/src/pages/subscription-management/index.ts
Outdated
Show resolved
Hide resolved
libs/email-subscriptions/src/pages/subscription-management/index.ts
Outdated
Show resolved
Hide resolved
libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts
Outdated
Show resolved
Hide resolved
🎭 Playwright E2E Test Results402 tests 402 ✅ 32m 56s ⏱️ Results for commit d54f4e9. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/email-subscriptions/src/subscription/service.test.ts (1)
267-274: Fix async mock to usemockResolvedValue.Line 271 uses
mockReturnValue(false)for the async functionvalidateLocationId, which should return a Promise. This will cause the test to fail when the service code is corrected to properly await the validation.Apply this diff:
vi.mocked(queries.findSubscriptionsByUserId).mockResolvedValue([]); - vi.mocked(validation.validateLocationId).mockReturnValue(false); + vi.mocked(validation.validateLocationId).mockResolvedValue(false); await expect(replaceUserSubscriptions(userId, newLocationIds)).rejects.toThrow("Invalid location ID"); });Note: This test will expose a bug in the service code. The
replaceUserSubscriptionsfunction inservice.ts(around line 86) does not awaitvalidateLocationId, unlikecreateSubscriptionwhich correctly awaits it. The service should be updated to:const locationValid = await validateLocationId(locationId);
🧹 Nitpick comments (10)
libs/email-subscriptions/src/pages/subscription-confirmed/index.test.ts (2)
43-55: Good coverage of happy path; consider mirroring cleanup checks in other casesThis test nicely checks:
- Correct locations passed to the view.
isPluralhandling for multiple locations.- Session cleanup (
confirmationCompleteandconfirmedLocationscleared).If the intended behaviour is “always clear confirmation state once the page is rendered”, consider adding similar cleanup expectations in the single-location and empty-locations tests so regressions on those paths are caught too.
57-99: Tighten assertions for redirect and edge casesThe branch coverage is good, but you could strengthen a few expectations:
- Redirect test (Lines 57–63): optionally assert that
renderwas not called (e.g.expect(mockRes.render).not.toHaveBeenCalled()), to guard against accidental double-response behaviour.- Single/empty confirmed locations (Lines 65–99): if the handler is expected to treat these similarly to the multi-location case (e.g. clear confirmation state, set
isPluralfor 0 locations), consider asserting those invariants too.These would make the tests more robust to subtle behavioural changes without adding much noise.
libs/email-subscriptions/src/pages/delete-subscription/index.test.ts (3)
13-24: Consider using proper session typing instead ofas any.The session is cast to
as any, which bypasses TypeScript's type checking and could hide potential type-related issues. Based on the module's session-types.ts, there should be a proper session type available.Consider importing and using the proper session type:
+import type { EmailSubscriptionsSession } from "../../session-types.js"; + describe("delete-subscription", () => { let mockReq: Partial<Request>; let mockRes: Partial<Response>; beforeEach(() => { mockReq = { query: {}, body: {}, - session: {} as any + session: { emailSubscriptions: {} } as any as Request['session'] };
68-76: Remove redundant session initialization.Line 70 re-initializes the session, but it's already initialized in the
beforeEachhook at line 17. This is redundant and can be removed.it("should store subscription in session and redirect if user selects yes", async () => { mockReq.body = { subscription: "sub123", "unsubscribe-confirm": "yes" }; - mockReq.session = {} as any; await POST[0](mockReq as Request, mockRes as Response, vi.fn());
30-85: Consider adding negative assertions for more robust tests.The tests verify the expected behavior (redirect or render is called) but don't verify that the alternative action is NOT called. For example, when testing redirects, you could also assert that
renderwas not called.Example enhancement for the first GET test:
it("should redirect if no subscriptionId provided", async () => { await GET[0](mockReq as Request, mockRes as Response, vi.fn()); expect(mockRes.redirect).toHaveBeenCalledWith("/subscription-management"); + expect(mockRes.render).not.toHaveBeenCalled(); });This pattern can be applied to other test cases where you're verifying redirects vs. renders.
libs/email-subscriptions/src/pages/location-name-search/index.test.ts (2)
43-71: Strengthen assertions to verify view data.The GET tests only assert that
renderwas called but don't verify the actual data passed to the template. This reduces test coverage effectiveness.Consider enhancing assertions to verify:
- The structure of data passed to the view (jurisdictionItems, regionItems, etc.)
- That query parameters are correctly transformed and passed
- Navigation items are properly built
Example for the jurisdiction filter test:
it("should handle jurisdiction filter", async () => { mockReq.query = { jurisdiction: "1" }; await GET[0](mockReq as Request, mockRes as Response, vi.fn()); - expect(mockRes.render).toHaveBeenCalled(); + expect(mockRes.render).toHaveBeenCalledWith( + "location-name-search/index", + expect.objectContaining({ + selectedJurisdiction: "1", + jurisdictionItems: expect.any(Array), + // ... other expected properties + }) + ); });
75-103: Consider adding validation and error scenario tests.The POST tests cover happy paths but miss validation and error scenarios such as:
- Invalid locationIds format (e.g., non-numeric strings, SQL injection patterns)
- Handling when session initialization fails
- Validation errors that should be displayed to the user
Example test:
it("should handle invalid location ID format", async () => { mockReq.body = { locationIds: "invalid-format" }; mockReq.session = {} as any; await POST[0](mockReq as Request, mockRes as Response, vi.fn()); // Verify error handling or sanitization expect(mockRes.render).toHaveBeenCalledWith( "location-name-search/index", expect.objectContaining({ errors: expect.any(Object) }) ); });libs/email-subscriptions/src/pages/subscription-management/index.test.ts (1)
43-77: Add tests for locale-based name selection and error handling.The tests verify location names but don't cover:
- Locale-based name selection (Welsh vs. English) mentioned in the AI summary
- Error handling when
getLocationByIdorgetSubscriptionsByUserIdfailsExample tests:
it("should use Welsh names when locale is cy", async () => { mockRes.locals = { locale: "cy" }; vi.mocked(subscriptionService.getSubscriptionsByUserId).mockResolvedValue([ { subscriptionId: "sub1", userId: "user123", locationId: "456", dateAdded: new Date() } ]); await GET[0](mockReq as Request, mockRes as Response, vi.fn()); expect(mockRes.render).toHaveBeenCalledWith( "subscription-management/index", expect.objectContaining({ subscriptions: expect.arrayContaining([ expect.objectContaining({ locationName: "Lleoliad 456" }) ]) }) ); }); it("should handle error when fetching subscriptions fails", async () => { vi.mocked(subscriptionService.getSubscriptionsByUserId).mockRejectedValue( new Error("Database error") ); await GET[0](mockReq as Request, mockRes as Response, vi.fn()); // Verify error is handled appropriately });libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts (1)
93-153: Consider adding tests for edge cases and session cleanup.The POST tests provide good coverage but could be enhanced with:
- Invalid or unknown action values (e.g.,
action: "invalid")- Verifying that
pendingSubscriptionsis cleared from session after successful confirmation- Testing removal of a locationId that doesn't exist in the pending list
Example tests:
it("should handle unknown action gracefully", async () => { mockReq.body = { action: "unknown" }; await POST[0](mockReq as Request, mockRes as Response, vi.fn()); // Verify appropriate error handling or default behavior }); it("should clear pendingSubscriptions after successful confirmation", async () => { mockReq.body = { action: "confirm" }; vi.mocked(subscriptionService.replaceUserSubscriptions).mockResolvedValue({ added: 2, removed: 0 }); await POST[0](mockReq as Request, mockRes as Response, vi.fn()); expect(mockReq.session?.emailSubscriptions?.pendingSubscriptions).toBeUndefined(); // or expect it to be cleared/reset }); it("should handle removing non-existent locationId", async () => { mockReq.body = { action: "remove", locationId: "999" }; await POST[0](mockReq as Request, mockRes as Response, vi.fn()); // Verify it doesn't break and handles gracefully expect(mockReq.session?.emailSubscriptions?.pendingSubscriptions).toEqual(["456", "789"]); });libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts (1)
49-62: Optionally strengthen assertions on the rendered view contextThe success-path test nicely checks the service call, session cleanup, and that the confirmation template is rendered. To catch more regressions in the future, you could also assert key parts of the render context (e.g. navigation/verifiedItems or translated copy) rather than only
expect.any(Object).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/email-subscriptions/src/pages/delete-subscription/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-confirmed/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts(1 hunks)libs/email-subscriptions/src/subscription/service.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/email-subscriptions/src/subscription/service.test.ts (1)
libs/email-subscriptions/src/subscription/service.ts (5)
createSubscription(13-30)getSubscriptionsByUserId(32-34)removeSubscription(36-50)createMultipleSubscriptions(52-63)replaceUserSubscriptions(65-97)
⏰ 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 (10)
libs/email-subscriptions/src/pages/subscription-confirmed/index.test.ts (1)
1-40: Solid test scaffolding and mocking setupThe structure is clear: shared
mockReq/mockResinbeforeEach, consistent use ofvi.fn()forrender/redirect, andvi.clearAllMocks()inafterEachkeeps tests isolated. Nothing blocking here.libs/email-subscriptions/src/pages/delete-subscription/index.test.ts (1)
1-8: LGTM!The imports and mock setup are clean and appropriate for testing the delete-subscription page handlers.
libs/email-subscriptions/src/subscription/service.test.ts (5)
1-17: LGTM!The imports and mock setup are well-structured. The mocks cover all the necessary dependencies for testing the subscription service in isolation.
28-77: Comprehensive test coverage for createSubscription.The test cases cover all critical paths: successful creation, invalid location validation, duplicate subscription detection, and subscription limit enforcement. All async mocks are correctly configured with
mockResolvedValue.
79-139: Well-tested retrieval and removal operations.The test suite properly validates:
- Subscription retrieval by user ID
- Successful removal with proper authorization
- Error handling for missing subscriptions and unauthorized access
The ownership verification test (lines 127-138) is particularly important for security.
141-185: Effective testing of batch operations.The tests properly verify both full-success and partial-failure scenarios using
mockResolvedValueOnceto simulate different validation outcomes. The error aggregation in the result structure is correctly validated.
190-265: Thorough coverage of subscription replacement scenarios.The test suite comprehensively validates:
- Mixed add/remove operations with correct delta tracking
- Edge cases: empty existing list, empty new list, no changes
- Boundary validation: max subscription limit enforcement
- Proper function call verification for creates and deletes
The test on lines 251-265 is particularly valuable for validating the subscription limit logic with realistic data (48 existing + 3 new = 51 > 50).
Also applies to: 276-291
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts (1)
102-114: Verify the business rule preventing removal of the last location.The test expects an error when removing the last pending location. This prevents users from having an empty pending subscriptions list.
Is this constraint intentional? Users might want to clear all selections and start over. If this is a deliberate business requirement, consider whether it provides the best user experience.
libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts (2)
39-48: Good guard-path test for missingsubscriptionToRemoveThe first test cleanly verifies the guard behavior when
subscriptionToRemoveis absent: it asserts the redirect and ensuresremoveSubscriptionis not called, which is exactly the right contract to lock in.
64-85: Solid coverage of error and user-id fallback pathsThe error test correctly asserts logging plus redirect, and the final test validates the
"test-user-id"fallback whenreq.useris missing, which are important edge cases for this route.
libs/email-subscriptions/src/pages/subscription-management/index.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
libs/email-subscriptions/src/pages/subscription-management/index.test.ts (1)
79-86: Test validates problematic production behavior.This test confirms that the production handler falls back to "test-user-id" when no user is present. As previously flagged, this test-specific logic should be removed from production code and replaced with proper authentication handling (401/redirect).
libs/email-subscriptions/src/subscription/repository/service.test.ts (1)
267-274: UsemockResolvedValuefor async function mock.Line 271 uses
mockReturnValue(false)for the asyncvalidateLocationIdfunction. Since the service awaits this function, it should return a Promise.vi.mocked(queries.findSubscriptionsByUserId).mockResolvedValue([]); - vi.mocked(validation.validateLocationId).mockReturnValue(false); + vi.mocked(validation.validateLocationId).mockResolvedValue(false);libs/email-subscriptions/src/pages/pending-subscriptions/index.ts (2)
58-68: Session mutation assumes initialized object after optional-chaining read.Line 65 reads with optional chaining (
req.session.emailSubscriptions?.pendingSubscriptions), but line 68 directly mutatesreq.session.emailSubscriptions.pendingSubscriptionswithout checking ifemailSubscriptionsexists. This will throw if the session object isn't initialized.This aligns with the past review comment about defensive session handling.
61-62: Auth TODOs must be resolved before release.The hard-coded
"test-user-id"fallback (line 62) and missing auth middleware (lines 139-140) were flagged in previous reviews. These must be addressed before merging to prevent writes against synthetic users.Also applies to: 139-141
🧹 Nitpick comments (7)
libs/email-subscriptions/src/config.test.ts (2)
5-16: pageRoutes tests look good; consider slightly stronger contract checksThe tests correctly assert that
pageRoutes.pathexists and is a string. If you want to tighten the contract a bit, you could:
- Assert that
pageRouteshas any other expected properties (e.g.router) rather than onlypath.- Use
path.basename(pageRoutes.path)(orpath.normalize) to assert the final directory name is"pages"instead of a loose substring match.Not required, but it would make failures a bit more targeted if the config shape changes.
29-42: prismaSchemas tests are solid; you could assert directory name more preciselyThese tests correctly verify that
prismaSchemasis a string, absolute, and includes"prisma", which aligns with thepath.join(__dirname, "../prisma")export inconfig.ts. For slightly clearer intent, you could replace the substring check with something like:expect(path.basename(prismaSchemas)).toBe("prisma");This keeps the contract explicit about the directory name while remaining implementation-agnostic.
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts (1)
92-154: Consider adding test for unknown/missing action values.The POST tests cover
removeandconfirmactions, but there's no test for what happens whenactionis missing or has an unexpected value. Given the implementation inindex.tsdoesn't handle this case (as noted in past reviews), adding a test would help document the expected behavior once fixed.it("should handle unknown action gracefully", async () => { mockReq.body = { action: "unknown" }; await POST[0](mockReq as Request, mockRes as Response, vi.fn()); // Expect redirect back or 400 response once implemented expect(mockRes.redirect).toHaveBeenCalledWith("/pending-subscriptions"); });libs/email-subscriptions/src/pages/pending-subscriptions/index.ts (2)
31-41: Extract duplicate location-mapping logic.The location mapping logic is duplicated between the GET handler (lines 31-41) and the POST error handler (lines 106-116). Consider extracting this into a helper function.
+const mapPendingLocations = (locationIds: string[], locale: string) => + locationIds + .map((id: string) => { + const location = getLocationById(Number.parseInt(id, 10)); + return location + ? { + locationId: id, + name: locale === "cy" ? location.welshName : location.name + } + : null; + }) + .filter(Boolean); + const getHandler = async (req: Request, res: Response) => { // ... - const pendingLocations = pendingLocationIds - .map((id: string) => { - const location = getLocationById(Number.parseInt(id, 10)); - return location - ? { - locationId: id, - name: locale === "cy" ? location.welshName : location.name - } - : null; - }) - .filter(Boolean); + const pendingLocations = mapPendingLocations(pendingLocationIds, locale);Also applies to: 106-116
15-18: Consider extracting repeated navigation setup.The navigation initialization pattern is repeated four times. A small helper or middleware could reduce this duplication.
const setupNavigation = (res: Response, path: string, locale: string) => { if (!res.locals.navigation) { res.locals.navigation = {}; } res.locals.navigation.verifiedItems = buildVerifiedUserNavigation(path, locale); };Also applies to: 43-46, 71-74, 118-121
libs/email-subscriptions/src/subscription/repository/service.ts (2)
36-50: Use query helper for consistency.
removeSubscriptionusesprisma.subscription.findUniquedirectly (line 37-39), while other functions use the imported query helpers. Consider usingfindSubscriptionByUserAndLocationor adding afindSubscriptionByIdhelper to maintain consistency.Add to
queries.ts:export async function findSubscriptionById(subscriptionId: string) { return prisma.subscription.findUnique({ where: { subscriptionId } }); }Then update
service.ts:-import { prisma } from "@hmcts/postgres"; import { countSubscriptionsByUserId, createSubscriptionRecord, deleteSubscriptionRecord, findSubscriptionByUserAndLocation, - findSubscriptionsByUserId + findSubscriptionsByUserId, + findSubscriptionById } from "./queries.js"; export async function removeSubscription(subscriptionId: string, userId: string) { - const subscription = await prisma.subscription.findUnique({ - where: { subscriptionId } - }); + const subscription = await findSubscriptionById(subscriptionId);
55-62: Redundant status check in error mapping.The
failedarray only contains rejected promises (filtered on line 56), so the conditionalr.status === "rejected"on line 61 is always true.- errors: failed.map((r) => (r.status === "rejected" ? r.reason.message : "Unknown error")) + errors: failed.map((r) => (r as PromiseRejectedResult).reason.message)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/postgres/prisma/migrations/20251124104824_add_email_subscriptions/migration.sql(1 hunks)libs/email-subscriptions/src/config.test.ts(1 hunks)libs/email-subscriptions/src/index.test.ts(1 hunks)libs/email-subscriptions/src/index.ts(1 hunks)libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/pending-subscriptions/index.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.ts(1 hunks)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts(1 hunks)libs/email-subscriptions/src/session-types.test.ts(1 hunks)libs/email-subscriptions/src/subscription/repository/queries.test.ts(1 hunks)libs/email-subscriptions/src/subscription/repository/queries.ts(1 hunks)libs/email-subscriptions/src/subscription/repository/service.test.ts(1 hunks)libs/email-subscriptions/src/subscription/repository/service.ts(1 hunks)libs/email-subscriptions/src/subscription/validation.test.ts(1 hunks)libs/email-subscriptions/src/subscription/validation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/email-subscriptions/src/subscription/validation.test.ts
- libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts
- libs/email-subscriptions/src/subscription/validation.ts
- libs/email-subscriptions/src/pages/subscription-management/index.ts
- libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts
- libs/email-subscriptions/src/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 should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/email-subscriptions/src/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.tslibs/email-subscriptions/src/subscription/repository/queries.tslibs/email-subscriptions/src/subscription/repository/queries.test.tslibs/email-subscriptions/src/config.test.tslibs/email-subscriptions/src/subscription/repository/service.test.tslibs/email-subscriptions/src/subscription/repository/service.tslibs/email-subscriptions/src/pages/subscription-management/index.test.tslibs/email-subscriptions/src/session-types.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/email-subscriptions/src/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.tslibs/email-subscriptions/src/subscription/repository/queries.tslibs/email-subscriptions/src/subscription/repository/queries.test.tslibs/email-subscriptions/src/config.test.tslibs/email-subscriptions/src/subscription/repository/service.test.tslibs/email-subscriptions/src/subscription/repository/service.tslibs/email-subscriptions/src/pages/subscription-management/index.test.tslibs/email-subscriptions/src/session-types.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All packages must use
"test": "vitest run"as the test script. Unit and integration tests must be co-located with source code as*.test.tsfiles.
Files:
libs/email-subscriptions/src/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.tslibs/email-subscriptions/src/subscription/repository/queries.test.tslibs/email-subscriptions/src/config.test.tslibs/email-subscriptions/src/subscription/repository/service.test.tslibs/email-subscriptions/src/pages/subscription-management/index.test.tslibs/email-subscriptions/src/session-types.test.ts
**/{pages,locales}/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
Every page must support both English and Welsh by providing
enandcyobjects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Files:
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.tslibs/email-subscriptions/src/pages/subscription-management/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Page routes are created based on file names within the
pages/directory. Nested routes are created using subdirectories (e.g.,pages/admin/my-page.tsbecomes/admin/my-page).
Files:
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.tslibs/email-subscriptions/src/pages/subscription-management/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
**/{locales,pages}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Files:
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.tslibs/email-subscriptions/src/pages/subscription-management/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
🧠 Learnings (6)
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to libs/*/src/index.ts : All modules must have `src/index.ts` for business logic exports separate from `src/config.ts`.
Applied to files:
libs/email-subscriptions/src/index.test.tslibs/email-subscriptions/src/config.test.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.
Applied to files:
libs/email-subscriptions/src/index.test.tslibs/email-subscriptions/src/subscription/repository/queries.test.tslibs/email-subscriptions/src/config.test.tslibs/email-subscriptions/src/pages/subscription-management/index.test.tslibs/email-subscriptions/src/session-types.test.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Applied to files:
libs/email-subscriptions/src/index.test.tslibs/email-subscriptions/src/config.test.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.
Applied to files:
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/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 must import config using the `/config` path (e.g., `hmcts/my-feature/config`).
Applied to files:
libs/email-subscriptions/src/config.test.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{locales,pages}/**/*.ts : Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Applied to files:
libs/email-subscriptions/src/pages/pending-subscriptions/index.ts
🧬 Code graph analysis (6)
libs/email-subscriptions/src/subscription/repository/queries.test.ts (1)
libs/email-subscriptions/src/subscription/repository/queries.ts (5)
findSubscriptionsByUserId(3-12)findSubscriptionByUserAndLocation(14-23)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)deleteSubscriptionRecord(42-46)
libs/email-subscriptions/src/config.test.ts (1)
libs/email-subscriptions/src/config.ts (1)
prismaSchemas(9-9)
libs/email-subscriptions/src/subscription/repository/service.test.ts (1)
libs/email-subscriptions/src/subscription/repository/service.ts (5)
createSubscription(13-30)getSubscriptionsByUserId(32-34)removeSubscription(36-50)createMultipleSubscriptions(52-63)replaceUserSubscriptions(65-97)
libs/email-subscriptions/src/subscription/repository/service.ts (2)
libs/email-subscriptions/src/subscription/validation.ts (1)
validateLocationId(4-7)libs/email-subscriptions/src/subscription/repository/queries.ts (5)
findSubscriptionByUserAndLocation(14-23)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)findSubscriptionsByUserId(3-12)deleteSubscriptionRecord(42-46)
libs/email-subscriptions/src/pages/subscription-management/index.test.ts (3)
libs/email-subscriptions/src/pages/pending-subscriptions/index.ts (1)
GET(140-140)libs/email-subscriptions/src/pages/subscription-management/index.ts (1)
GET(37-37)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts (1)
GET(39-39)
libs/email-subscriptions/src/pages/pending-subscriptions/index.ts (3)
libs/email-subscriptions/src/pages/pending-subscriptions/cy.ts (1)
cy(1-12)libs/email-subscriptions/src/pages/pending-subscriptions/en.ts (1)
en(1-12)libs/email-subscriptions/src/subscription/repository/service.ts (1)
replaceUserSubscriptions(65-97)
⏰ 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)
apps/postgres/prisma/migrations/20251124104824_add_email_subscriptions/migration.sql (1)
3-6: Verify column types and UUID generation strategy.The columns
user_idandlocation_idare defined asTEXTrather than typed references (e.g.,UUID), andsubscription_idhas noDEFAULTvalue for UUID generation. Verify that:
- The application layer correctly generates UUIDs for
subscription_idbefore insert (or add aDEFAULT gen_random_uuid()if using PostgreSQL-side generation).user_idandlocation_idasTEXTfields is intentional (e.g., storing external system IDs) and is aligned with the Prisma schema and application layer.libs/email-subscriptions/src/config.test.ts (1)
18-27: moduleRoot tests are clear and sufficientAsserting that
moduleRootis a defined string and an absolute path is exactly the right level of contract here for consumers like the web app and schema discovery. This nicely complements theconfig.tsexport pattern described in the repo learnings.libs/email-subscriptions/src/session-types.test.ts (1)
1-60: LGTM! Type augmentation tests are well-structured.These tests effectively validate the TypeScript type extensions for
SessionData. The approach of using compile-time type checking to verify the augmented properties (emailSubscriptions,successMessage) is appropriate for this use case.libs/email-subscriptions/src/pages/subscription-management/index.test.ts (1)
1-77: Test structure and mocking patterns look good.The test setup properly mocks external dependencies (
@hmcts/auth,@hmcts/location, subscription service) and validates the GET handler's rendering behavior with subscriptions data enriched with location names.libs/email-subscriptions/src/index.test.ts (1)
1-25: LGTM! Public API surface tests are appropriate.These smoke tests validate that the expected functions are exported from the module's public API. The tests serve as documentation of the module's contract and will catch accidental removals of exports.
libs/email-subscriptions/src/subscription/repository/service.test.ts (3)
1-77: Comprehensive test coverage for createSubscription.Tests properly cover the success path and all error conditions: invalid location, duplicate subscription, and subscription limit. Mock setup is clean and assertions verify both return values and function calls.
100-139: LGTM! removeSubscription tests cover authorization correctly.Good coverage of the authorization check - verifying that users cannot remove subscriptions they don't own. The "Unauthorized" error test (lines 127-138) is particularly important for security.
187-292: replaceUserSubscriptions tests are thorough.Good coverage of the various scenarios: add/remove combinations, empty lists, max subscription limits, and idempotent behavior when lists match. The edge case at lines 251-264 correctly tests the boundary condition for max subscriptions.
libs/email-subscriptions/src/subscription/repository/queries.test.ts (4)
1-31: LGTM! Clean test setup with proper Prisma mocking.The mock structure correctly mirrors the Prisma client interface, and the lifecycle hooks ensure clean state between tests.
32-74: Good coverage for findSubscriptionsByUserId.Tests verify both the return value and the exact query parameters including the
orderBy: { dateAdded: "desc" }clause, ensuring subscriptions are returned in the expected order.
76-112: Unique constraint query test is correct.The test validates that
findSubscriptionByUserAndLocationuses the composite unique constraintunique_user_locationproperly, which aligns with the Prisma schema.
114-185: LGTM! Remaining query tests are thorough.Tests for count, create, and delete operations properly verify the Prisma method calls and return values. Good coverage of the zero-count edge case.
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts (1)
1-46: LGTM!Test setup is well-structured with proper mocking of dependencies (
@hmcts/auth,@hmcts/location, subscription service) and clean mock reset inafterEach. The mock request/response objects appropriately simulate the Express request lifecycle.libs/email-subscriptions/src/subscription/repository/queries.ts (1)
1-46: LGTM!Clean data access layer implementation following the repository pattern. Functions are appropriately thin wrappers around Prisma queries with clear single responsibilities. Error handling is correctly delegated to the service layer.
libs/email-subscriptions/src/subscription/repository/service.ts (1)
13-30: LGTM on validation and business logic.The
createSubscriptionfunction properly validates location, checks for duplicates, and enforces the max subscription limit. ThereplaceUserSubscriptionsfunction correctly computes diffs and enforces limits. The past issue with missingawaitonvalidateLocationIdhas been addressed.Also applies to: 65-97
apps/postgres/prisma/migrations/20251124104824_add_email_subscriptions/migration.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
libs/email-subscriptions/src/pages/pending-subscriptions/index.ts (3)
61-62: Remove test user fallback before release.The
"test-user-id"fallback on line 62 persists from earlier review feedback. This risks writing subscriptions against a synthetic user and must not ship to production.- // TODO: Remove this mock user ID - for testing only - const userId = req.user?.id || "test-user-id"; + const userId = req.user?.id; + if (!userId) { + return res.redirect("/"); + }
65-68: Guard against undefined session before mutation.Line 65 reads
pendingSubscriptionswith optional chaining, but line 68 directly assigns toreq.session.emailSubscriptions.pendingSubscriptionswithout verifyingemailSubscriptionsexists. This can throw if the session object isn't initialized.const pendingLocationIds = req.session.emailSubscriptions?.pendingSubscriptions || []; if (action === "remove" && locationId) { + if (!req.session.emailSubscriptions) { + req.session.emailSubscriptions = { pendingSubscriptions: [] }; + } req.session.emailSubscriptions.pendingSubscriptions = pendingLocationIds.filter((id: string) => id !== locationId);
90-137: Add default handling for unknownactionvalues.If
actionis neither"remove"nor"confirm", the handler completes without sending a response or callingnext(), leaving the request hanging. Add a fallback to redirect or return an error.res.render("pending-subscriptions/index", { ...t, errors: { titleText: t.errorSummaryTitle, errorList: [{ text: errorMessage }] }, locations: pendingLocations, isPlural, confirmButton: isPlural ? t.confirmButtonPlural : t.confirmButton }); } } + + // Default case: redirect back if action is unknown or missing + return res.redirect("/pending-subscriptions"); };libs/email-subscriptions/src/repository/service.test.ts (1)
267-274: Optional: make async mock style consistent forvalidateLocationIdHere
validateLocationIdis mocked withmockReturnValue(false)while other tests usemockResolvedValue(...). Both work at runtime (sinceawaitaccepts plain values), but switching this tomockResolvedValue(false)would keep the mocking style consistent across the suite and with the function’s async signature.libs/email-subscriptions/src/pages/delete-subscription/index.ts (1)
56-93: POST should validate subscription ID and ownership before storing in sessionThe POST handler currently trusts
subscriptionfrom the body wheneverunsubscribe-confirm === "yes"and writes it into the session without any validation:if (unsubscribeConfirm === "yes" && subscription) { req.session.emailSubscriptions = req.session.emailSubscriptions || {}; req.session.emailSubscriptions.subscriptionToRemove = subscription; return res.redirect("/unsubscribe-confirmation"); }An attacker can POST arbitrary IDs here (including invalid UUIDs or subscriptions owned by another user). While the downstream removal service does enforce ownership, this still violates the “validate endpoint inputs” guideline and can surface as confusing 5xx errors when deletion is attempted. As per earlier review feedback, this should be tightened.
You can reuse the existing UUID validator and repository helper to guard this path:
const postHandler = async (req: Request, res: Response) => { const locale = res.locals.locale || "en"; const t = locale === "cy" ? cy : en; const { subscription, "unsubscribe-confirm": unsubscribeConfirm } = req.body; @@ if (unsubscribeConfirm === "yes" && subscription) { - req.session.emailSubscriptions = req.session.emailSubscriptions || {}; - req.session.emailSubscriptions.subscriptionToRemove = subscription; - return res.redirect("/unsubscribe-confirmation"); + if (!isValidUUID(subscription)) { + return res.redirect("/subscription-management"); + } + + const userId = req.user?.id; + if (!userId) { + return res.redirect("/subscription-management"); + } + + try { + const subRecord = await findSubscriptionById(subscription); + if (!subRecord || subRecord.userId !== userId) { + return res.redirect("/subscription-management"); + } + } catch (error) { + console.error("Error validating subscription for delete:", error); + return res.redirect("/subscription-management"); + } + + req.session.emailSubscriptions = req.session.emailSubscriptions || {}; + req.session.emailSubscriptions.subscriptionToRemove = subscription; + return res.redirect("/unsubscribe-confirmation"); }This keeps the session in sync with real, user-owned subscriptions and satisfies the per-endpoint validation requirement.
libs/email-subscriptions/src/repository/service.ts (1)
65-97: replaceUserSubscriptions can leave subscriptions in an inconsistent state
replaceUserSubscriptionscomputestoDeleteandtoAdd, checks the max-count constraint, then runs deletes and adds concurrently:await Promise.all([ ...toDelete.map((sub) => deleteSubscriptionRecord(sub.subscriptionId)), ...toAdd.map(async (locationId) => { const locationValid = await validateLocationId(locationId); if (!locationValid) { throw new Error(`Invalid location ID: ${locationId}`); } return createSubscriptionRecord(userId, locationId); }) ]);If any
validateLocationId/createSubscriptionRecordcall fails after some deletions have already succeeded, the promise rejects and the user is left with a partially updated set of subscriptions. Duplicates innewLocationIdscan also cause mid-flight failures (2nd create hits a uniqueness error) after deletions have completed.To avoid this partial-failure window, consider:
- Pre-validating all
toAddIDs before any mutation:if (toAdd.length > 0) { const currentCount = existingLocationIds.length - toDelete.length; if (currentCount + toAdd.length > MAX_SUBSCRIPTIONS) { throw new Error(`Maximum ${MAX_SUBSCRIPTIONS} subscriptions allowed`); } const validationResults = await Promise.all(toAdd.map((locId) => validateLocationId(locId))); const invalidIndex = validationResults.findIndex((ok) => !ok); if (invalidIndex !== -1) { throw new Error(`Invalid location ID: ${toAdd[invalidIndex]}`); } }
- Then performing mutations without further validation, ideally in a transaction:
await Promise.all([ ...toDelete.map((sub) => deleteSubscriptionRecord(sub.subscriptionId)), ...toAdd.map((locationId) => createSubscriptionRecord(userId, locationId)) ]);If your
@hmcts/postgresPrisma client exposes$transaction, wrapping the deletes and creates in a single transaction would give you full atomicity.This will make the replace operation predictable and avoid silently losing existing subscriptions on validation/create failures.
🧹 Nitpick comments (9)
libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts (1)
94-156: Good test coverage for POST handler, but missing test for unknownactionvalues.The tests comprehensively cover
removeandconfirmactions, including error scenarios. However, there's no test for whenactionis an unexpected value (e.g.,"invalid"orundefined). Based on past review comments, this was flagged as an issue in the implementation—adding a test would help catch if the handler leaves the request hanging.Consider adding a test case:
+ it("should handle unknown action gracefully", async () => { + mockReq.body = { action: "unknown" }; + + await POST[POST.length - 1](mockReq as Request, mockRes as Response, vi.fn()); + + // Verify response is sent (redirect or render) + expect(mockRes.redirect).toHaveBeenCalled(); + // or expect(mockRes.render).toHaveBeenCalled(); + });libs/email-subscriptions/src/validation/validation.test.ts (1)
1-69: Validation tests adequately cover main success/failure pathsThe tests for
validateLocationIdandvalidateDuplicateSubscriptionalign with the implementation and exercise both positive and negative branches, including non-numeric IDs and existing-subscription scenarios. If you want to be stricter about the “non‑numeric ID” behaviour, consider also asserting the expected interaction (or lack thereof) withgetLocationByIdin that case, but it’s not essential.libs/email-subscriptions/src/repository/service.test.ts (1)
141-185: Consider adding coverage for near-limit cases increateMultipleSubscriptions
createMultipleSubscriptionsis only tested for all‑success and partial‑failure (validation) scenarios. Given the globalMAX_SUBSCRIPTIONSrule enforced elsewhere in the service, it would be useful to add a test where the user already has a high subscription count and attempts to add multiple new locations, to lock in the desired behaviour when approaching or exceeding the limit.libs/email-subscriptions/src/pages/delete-subscription/index.njk (1)
60-85: Align client-side error styling more closely with GOV.UK form-group patternRight now the script adds
govuk-form-group--errortoradiosContainer.parentElement, which will be the immediate wrapper (e.g. a radios item) rather than the.govuk-form-group. Consider resolving the container viaconst formGroup = radiosContainer.closest(".govuk-form-group")and applying the class/inserted error there, and optionally moving focus to the error or first radio when validation fails to better support keyboard and screen-reader users.libs/email-subscriptions/src/pages/delete-subscription/index.test.ts (1)
1-159: Comprehensive delete-subscription coverage; small optional tidy-upThe suite gives good coverage of the GET/POST controller paths, including validation, ownership, and DB error handling, and the mocking strategy is sound.
If you want to tighten it further, you could:
- Factor small helpers like
runGet/runPostinstead of repeatingGET[GET.length - 1](...)andPOST[POST.length - 1](...).- Explicitly assert that
findSubscriptionByIdis not called whensubscriptionIdis missing/invalid (to lock in the short‑circuit behaviour).Both are non-essential polish; the tests are already in good shape.
libs/email-subscriptions/src/pages/subscription-confirmed/index.test.ts (1)
1-103: Solid tests for subscription-confirmed flowThese tests nicely pin down the core behaviours: redirect on missing confirmation, pluralisation, location name resolution, and session cleanup. They align well with the controller logic.
If you later want to extend coverage, you could add a simple
cylocale test (settingres.locals.locale = "cy") to lock in the Welsh panel title / name selection, but that’s optional.libs/email-subscriptions/src/validation/validation.ts (1)
1-12: Validation helpers look correct; consider minor clean-upBoth validators correctly wrap the underlying helpers and satisfy the “validate inputs before DB writes” guideline.
Two minor, optional tweaks:
- If
getLocationByIdis guaranteed sync (as implied elsewhere),validateLocationIddoesn’t need to beasync; you can returnPromise.resolvefrom callers instead, or leave as-is for a uniform async API.- You might defensively handle clearly invalid IDs (e.g.
Number.isNaN(Number.parseInt(locationId, 10))→false) to make intent explicit, although the current behaviour already returnsfalsefor invalid input.libs/email-subscriptions/src/pages/subscription-confirmed/index.ts (1)
1-42: Controller behaviour and auth chain look goodThis now correctly protects the page with
requireAuth()andblockUserAccess(), validates the confirmation flag, clears one-time session state, and builds verified navigation per the coding guidelines for pages/controllers.If you want to tighten typing, you could narrow
confirmedLocationsafter thefilterso TS knows it’sstring[], e.g.:const confirmedLocations = confirmedLocationIds .map((id: string) => { const location = getLocationById(Number.parseInt(id, 10)); return location ? (locale === "cy" ? location.welshName : location.name) : null; }) .filter((name): name is string => Boolean(name));But the current runtime behaviour is fine.
e2e-tests/tests/email-subscriptions.spec.ts (1)
1-618: End-to-end coverage is strong; consider reducing data-dependence in a few testsThis suite does a good job exercising the full email-subscriptions journeys (including auth protection, EN/CY content, and accessibility), and the login helper integration looks appropriate.
Two optional areas to tighten over time:
- Several tests gate their assertions on existing data, e.g. checking
const count = await deleteLinks.count(); if (count > 0) { ... }. In environments where no subscriptions exist, those tests effectively become no-ops. If feasible, explicitly create the required subscription(s) within the test setup so the assertions always run.- The invalid subscription ID test currently accepts “400 error or redirect to /subscription-management”. Now that the delete controller returns a 400, you could assert the specific expected behaviour for more deterministic checks.
These aren’t blockers; the current tests are already very valuable.
📜 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 (28)
e2e-tests/tests/account-home.spec.ts(3 hunks)e2e-tests/tests/email-subscriptions.spec.ts(1 hunks)libs/auth/src/middleware/navigation-helper.test.ts(2 hunks)libs/auth/src/middleware/navigation-helper.ts(1 hunks)libs/email-subscriptions/src/index.ts(1 hunks)libs/email-subscriptions/src/pages/delete-subscription/cy.ts(1 hunks)libs/email-subscriptions/src/pages/delete-subscription/en.ts(1 hunks)libs/email-subscriptions/src/pages/delete-subscription/index.njk(1 hunks)libs/email-subscriptions/src/pages/delete-subscription/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/delete-subscription/index.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.ts(1 hunks)libs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/pending-subscriptions/index.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-confirmed/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-confirmed/index.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.ts(1 hunks)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts(1 hunks)libs/email-subscriptions/src/repository/queries.test.ts(1 hunks)libs/email-subscriptions/src/repository/queries.ts(1 hunks)libs/email-subscriptions/src/repository/service.test.ts(1 hunks)libs/email-subscriptions/src/repository/service.ts(1 hunks)libs/email-subscriptions/src/validation/validation.test.ts(1 hunks)libs/email-subscriptions/src/validation/validation.ts(1 hunks)libs/verified-pages/src/pages/account-home/index.njk(1 hunks)libs/verified-pages/src/pages/account-home/index.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/email-subscriptions/src/pages/subscription-management/index.ts
- libs/email-subscriptions/src/pages/location-name-search/index.ts
- libs/email-subscriptions/src/pages/delete-subscription/en.ts
- libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts
- libs/email-subscriptions/src/pages/location-name-search/index.test.ts
- libs/email-subscriptions/src/pages/subscription-management/index.test.ts
- libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts
🧰 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 should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/auth/src/middleware/navigation-helper.test.tslibs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.tslibs/auth/src/middleware/navigation-helper.tslibs/email-subscriptions/src/repository/queries.test.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.test.tslibs/email-subscriptions/src/validation/validation.tse2e-tests/tests/account-home.spec.tslibs/email-subscriptions/src/repository/service.tslibs/email-subscriptions/src/pages/delete-subscription/cy.tse2e-tests/tests/email-subscriptions.spec.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.tslibs/email-subscriptions/src/repository/service.test.tslibs/email-subscriptions/src/validation/validation.test.tslibs/email-subscriptions/src/index.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.tslibs/email-subscriptions/src/repository/queries.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/auth/src/middleware/navigation-helper.test.tslibs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.tslibs/auth/src/middleware/navigation-helper.tslibs/email-subscriptions/src/repository/queries.test.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.test.tslibs/email-subscriptions/src/validation/validation.tse2e-tests/tests/account-home.spec.tslibs/email-subscriptions/src/repository/service.tslibs/email-subscriptions/src/pages/delete-subscription/cy.tse2e-tests/tests/email-subscriptions.spec.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.tslibs/email-subscriptions/src/repository/service.test.tslibs/email-subscriptions/src/validation/validation.test.tslibs/email-subscriptions/src/index.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.tslibs/email-subscriptions/src/repository/queries.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All packages must use
"test": "vitest run"as the test script. Unit and integration tests must be co-located with source code as*.test.tsfiles.
Files:
libs/auth/src/middleware/navigation-helper.test.tslibs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/repository/queries.test.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.test.tslibs/email-subscriptions/src/repository/service.test.tslibs/email-subscriptions/src/validation/validation.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts
**/{pages,locales}/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
Every page must support both English and Welsh by providing
enandcyobjects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Files:
libs/verified-pages/src/pages/account-home/index.njklibs/email-subscriptions/src/pages/delete-subscription/index.njklibs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/cy.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Page routes are created based on file names within the
pages/directory. Nested routes are created using subdirectories (e.g.,pages/admin/my-page.tsbecomes/admin/my-page).
Files:
libs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/cy.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts
**/{locales,pages}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Files:
libs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/cy.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.test.ts
libs/*/src/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All modules must have
src/index.tsfor business logic exports separate fromsrc/config.ts.
Files:
libs/email-subscriptions/src/index.ts
🧠 Learnings (9)
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.njk : Nunjucks templates must extend `layouts/default.njk` and use GOV.UK Design System macros. Every page must support both English and Welsh content.
Applied to files:
libs/email-subscriptions/src/pages/delete-subscription/index.njk
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{locales,pages}/**/*.ts : Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Applied to files:
libs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/cy.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{pages,locales}/**/*.{ts,njk} : Every page must support both English and Welsh by providing `en` and `cy` objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Applied to files:
libs/verified-pages/src/pages/account-home/index.test.tslibs/email-subscriptions/src/pages/delete-subscription/cy.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.
Applied to files:
libs/email-subscriptions/src/pages/delete-subscription/index.tslibs/email-subscriptions/src/pages/delete-subscription/cy.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.tslibs/email-subscriptions/src/pages/pending-subscriptions/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Applied to files:
libs/email-subscriptions/src/pages/delete-subscription/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.
Applied to files:
libs/email-subscriptions/src/repository/queries.test.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.test.tslibs/email-subscriptions/src/validation/validation.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.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/email-subscriptions/src/pages/delete-subscription/cy.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to libs/*/src/index.ts : All modules must have `src/index.ts` for business logic exports separate from `src/config.ts`.
Applied to files:
libs/email-subscriptions/src/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Applied to files:
libs/email-subscriptions/src/index.ts
🧬 Code graph analysis (8)
libs/auth/src/middleware/navigation-helper.test.ts (1)
libs/auth/src/middleware/navigation-helper.ts (1)
buildVerifiedUserNavigation(67-99)
libs/email-subscriptions/src/repository/queries.test.ts (1)
libs/email-subscriptions/src/repository/queries.ts (6)
findSubscriptionsByUserId(3-12)findSubscriptionByUserAndLocation(14-23)findSubscriptionById(42-46)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)deleteSubscriptionRecord(48-52)
libs/email-subscriptions/src/validation/validation.ts (1)
libs/email-subscriptions/src/repository/queries.ts (1)
findSubscriptionByUserAndLocation(14-23)
libs/email-subscriptions/src/repository/service.ts (2)
libs/email-subscriptions/src/validation/validation.ts (1)
validateLocationId(4-7)libs/email-subscriptions/src/repository/queries.ts (5)
findSubscriptionByUserAndLocation(14-23)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)findSubscriptionsByUserId(3-12)deleteSubscriptionRecord(48-52)
libs/email-subscriptions/src/pages/delete-subscription/cy.ts (3)
libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)
cy(1-23)libs/email-subscriptions/src/pages/unsubscribe-confirmation/cy.ts (1)
cy(1-11)libs/email-subscriptions/src/pages/subscription-management/cy.ts (1)
cy(1-10)
e2e-tests/tests/email-subscriptions.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
libs/email-subscriptions/src/repository/service.test.ts (1)
libs/email-subscriptions/src/repository/service.ts (5)
createSubscription(13-30)getSubscriptionsByUserId(32-34)removeSubscription(36-50)createMultipleSubscriptions(52-63)replaceUserSubscriptions(65-97)
libs/email-subscriptions/src/validation/validation.test.ts (1)
libs/email-subscriptions/src/validation/validation.ts (2)
validateLocationId(4-7)validateDuplicateSubscription(9-12)
⏰ 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/email-subscriptions/src/pages/pending-subscriptions/index.test.ts (1)
1-48: LGTM! Test setup and mocks are well-structured.The mock configuration properly isolates auth, location lookup, and subscription service dependencies. The
beforeEach/afterEachpattern ensures clean state between tests.libs/email-subscriptions/src/pages/pending-subscriptions/index.ts (2)
139-140: Auth middleware now correctly applied.The
requireAuth()andblockUserAccess()middleware are now included in bothGETandPOSTexports, addressing the previous review feedback about missing authentication protection.
8-56: LGTM! GET handler correctly implements locale-aware rendering.The handler properly:
- Selects Welsh/English translations based on locale
- Handles empty state with appropriate error messaging
- Maps location IDs to names with Welsh support
- Sets up navigation defensively
- Computes pluralization for button text
libs/auth/src/middleware/navigation-helper.test.ts (2)
102-103: LGTM! Test correctly updated to reflect new navigation path.The expectation change from
"/"to"/subscription-management"aligns with the implementation innavigation-helper.ts(lines 89-93) where the Email subscriptions link now points to/subscription-management.
115-119: LGTM! Current path test correctly updated.The test now passes
/subscription-managementto verify thecurrentflag is set correctly, matching the updated navigation implementation.libs/verified-pages/src/pages/account-home/index.njk (1)
31-38: LGTM! Email subscriptions tile href updated consistently.The href change from
"/"to"/subscription-management"aligns with the navigation helper updates and ensures the tile links to the new subscription management page.libs/email-subscriptions/src/pages/delete-subscription/cy.ts (1)
1-9: Welsh localization file verified with matching English structure.Both
cy.tsanden.tsexist with identical key structure (title, header, radio1, radio2, continueButton, errorSummaryTitle, errorNoSelection), ensuring proper locale parity across the delete-subscription page as required by coding guidelines.libs/email-subscriptions/src/index.ts (1)
1-2: Public API re-exports look correct and ESM-safeRe-exporting the service and validation modules via
.js-suffixed relative paths is consistent with the “nodenext”/ESM guidelines and keeps the index surface focused on actual business APIs.e2e-tests/tests/account-home.spec.ts (1)
153-172: Updated expectations to/subscription-managementare consistent with navigation changesThe href and navigation assertions for the Email subscriptions tile (both EN and CY) now correctly point to
/subscription-management, matching the updated verified-user navigation and routes.Also applies to: 398-405
libs/verified-pages/src/pages/account-home/index.test.ts (1)
14-21: Verified navigation mock correctly targets/subscription-managementThe test double for
buildVerifiedUserNavigationand the associated expectations now mirror the real helper’s/subscription-managementhref andcurrentlogic for the Email subscriptions item in both English and Welsh.Also applies to: 193-195, 207-211
libs/auth/src/middleware/navigation-helper.ts (1)
91-94: Verified-user nav now correctly routes Email subscriptions to/subscription-managementSwitching the Email subscriptions item to use
/subscription-management(both href andcurrentcheck) aligns navigation with the new subscriptions feature without affecting other roles.libs/email-subscriptions/src/repository/service.test.ts (1)
28-77: Service tests cover key business rules and error cases wellThe tests around
createSubscription,getSubscriptionsByUserId, andremoveSubscriptionthoroughly exercise happy paths and error branches (invalid location, duplicate subscription, limit reached, not found, unauthorized), and the prisma/query mocks are correctly scoped so the service logic is tested in isolation.Also applies to: 79-139
libs/email-subscriptions/src/repository/queries.test.ts (1)
1-218: Query helper tests are thorough and correctly assert Prisma interactionsEach repository function is exercised with realistic data and verified against the exact Prisma call signatures (filters, orderBy, where clauses), including empty/not‑found cases, which gives good confidence in the data-access layer.
libs/email-subscriptions/src/pages/delete-subscription/index.njk (1)
1-56: Template correctly wires layout, CSRF, error summary, and form controlsThe page now extends the base layout, uses GOV.UK macros, includes a hidden
_csrffield, and renders a propergovukErrorSummarywhenerrorsis present, with radios and button text driven by the localized variables—this aligns well with the design and i18n guidelines.libs/email-subscriptions/src/repository/queries.ts (1)
1-52: Prisma query helpers are straightforward and idiomaticThese helpers are thin, readable wrappers over Prisma and satisfy the parameterised-query requirement. Ordering of
findSubscriptionsByUserIdbydateAdded: "desc"and the dedicated helpers for uniqueness and delete-by-id keep higher layers clean.No changes needed here.
libs/email-subscriptions/src/pages/delete-subscription/index.ts
Outdated
Show resolved
Hide resolved
junaidiqbalmoj
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/email-subscriptions/src/repository/service.test.ts (1)
267-274: Past issue has been resolved.The previous review comment noted that line 279 incorrectly used
mockReturnValue(false)for the async functionvalidateLocationId. The current code at line 271 correctly usesmockResolvedValue(false), ensuring the mock returns a Promise.
🧹 Nitpick comments (6)
libs/email-subscriptions/src/pages/delete-subscription/index.test.ts (1)
21-35: Consider using proper session typing instead ofanycasts.Lines 25 and 143 cast session objects to
any, which bypasses TypeScript's type checking. While common in tests, this can hide type mismatches. Consider importing and using the actual session type from your session configuration.libs/email-subscriptions/src/repository/service.ts (1)
65-100: Past concern has been addressed - validation now happens upfront.The previous review flagged the risk of inconsistent state if validation failed during concurrent mutations. The code now correctly:
- Validates all location IDs upfront (lines 82-88)
- Performs mutations only after all validations pass (lines 91-94)
This ensures that deletions and creations won't leave partial state due to validation failures.
Optional: Consider database transactions for database-level failure resilience.
While upfront validation addresses the primary concern, database-level failures during the
Promise.allmutations could still leave inconsistent state. If Prisma transactions are available and the use case requires strict atomicity, wrapping lines 91-94 in a transaction would provide additional safety.libs/auth/src/middleware/navigation.ts (1)
18-27: Consider using a constant for the role comparison.The hardcoded string
"VERIFIED"on Line 21 should ideally be replaced with a constant for consistency and maintainability.Run the following script to verify if
USER_ROLES.VERIFIEDexists:#!/bin/bash # Check if USER_ROLES.VERIFIED is defined ast-grep --pattern 'export const USER_ROLES = { $$$ }'If
USER_ROLES.VERIFIEDexists, apply this diff:+import { USER_ROLES } from "@hmcts/account"; import type { NextFunction, Request, Response } from "express"; import { buildNavigationItems, buildVerifiedUserNavigation } from "./navigation-helper.js"; /** * Middleware to set navigation state based on authentication status * Sets res.locals.isAuthenticated and res.locals.navigation for global navigation * Navigation items are role-based and appear on all pages for authenticated users */ export function authNavigationMiddleware() { return (req: Request, res: Response, next: NextFunction) => { res.locals.isAuthenticated = req.isAuthenticated(); // Initialize navigation object if it doesn't exist if (!res.locals.navigation) { res.locals.navigation = {}; } // Add role-based navigation items for authenticated users if (req.isAuthenticated() && req.user?.role) { // For VERIFIED users (media users), show Dashboard and Email subscriptions - if (req.user.role === "VERIFIED") { + if (req.user.role === USER_ROLES.VERIFIED) { const locale = res.locals.locale || "en"; res.locals.navigation.verifiedItems = buildVerifiedUserNavigation(req.path, locale); } else { // For SSO admin roles, show admin navigation res.locals.navigation.verifiedItems = buildNavigationItems(req.user.role, req.path); } } else { // Clear navigation items when user is not authenticated or has no role res.locals.navigation.verifiedItems = undefined; } next(); }; }libs/auth/src/pages/cft-callback/index.ts (1)
30-47: LGTM with minor suggestion.The database interaction flow is correct, with proper error handling and exception tracking. However, consider extracting the hardcoded strings
"VERIFIED"(Line 38) and"CFT_IDAM"(Line 36) into constants for consistency with coding guidelines.libs/email-subscriptions/src/pages/location-name-search/index.ts (1)
147-180: LGTM with minor observation.The rendering logic is correct. Note that both
enandcycontent objects are passed to the template (Lines 162-163), thoughcontentis already resolved based on locale. This might be intentional for template-level locale switching, but consider if both are necessary.e2e-tests/tests/email-subscriptions.spec.ts (1)
30-96: LGTM with minor observation.Tests comprehensively cover the Subscription Management Page, including accessibility and Welsh language support. Note that Line 47 uses
.nth(2)to select the email subscriptions tile, which relies on tile order and may be brittle if the order changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/postgres/prisma/migrations/20251124104824_add_email_subscriptions/migration.sql(1 hunks)apps/postgres/prisma/schema.prisma(1 hunks)e2e-tests/tests/email-subscriptions.spec.ts(1 hunks)libs/auth/src/middleware/navigation.test.ts(1 hunks)libs/auth/src/middleware/navigation.ts(2 hunks)libs/auth/src/pages/cft-callback/index.test.ts(3 hunks)libs/auth/src/pages/cft-callback/index.ts(2 hunks)libs/email-subscriptions/prisma/schema.prisma(1 hunks)libs/email-subscriptions/src/pages/delete-subscription/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/delete-subscription/index.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/cy.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/en.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.njk(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.ts(1 hunks)libs/email-subscriptions/src/pages/pending-subscriptions/index.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.njk(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.ts(1 hunks)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts(1 hunks)libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts(1 hunks)libs/email-subscriptions/src/repository/service.test.ts(1 hunks)libs/email-subscriptions/src/repository/service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.ts
- libs/email-subscriptions/src/pages/location-name-search/en.ts
- libs/email-subscriptions/src/pages/location-name-search/cy.ts
- libs/email-subscriptions/src/pages/location-name-search/index.njk
- libs/email-subscriptions/src/pages/location-name-search/index.test.ts
- apps/postgres/prisma/migrations/20251124104824_add_email_subscriptions/migration.sql
- libs/email-subscriptions/src/pages/unsubscribe-confirmation/index.test.ts
- libs/email-subscriptions/src/pages/pending-subscriptions/index.ts
- libs/email-subscriptions/prisma/schema.prisma
- libs/email-subscriptions/src/pages/subscription-management/index.test.ts
- libs/email-subscriptions/src/pages/subscription-management/index.njk
- libs/email-subscriptions/src/pages/subscription-management/index.ts
- libs/email-subscriptions/src/pages/delete-subscription/index.ts
🧰 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 should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/auth/src/middleware/navigation.test.tslibs/email-subscriptions/src/repository/service.test.tslibs/auth/src/pages/cft-callback/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/auth/src/middleware/navigation.tslibs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/repository/service.tse2e-tests/tests/email-subscriptions.spec.tslibs/auth/src/pages/cft-callback/index.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/auth/src/middleware/navigation.test.tslibs/email-subscriptions/src/repository/service.test.tslibs/auth/src/pages/cft-callback/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/auth/src/middleware/navigation.tslibs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/repository/service.tse2e-tests/tests/email-subscriptions.spec.tslibs/auth/src/pages/cft-callback/index.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All packages must use
"test": "vitest run"as the test script. Unit and integration tests must be co-located with source code as*.test.tsfiles.
Files:
libs/auth/src/middleware/navigation.test.tslibs/email-subscriptions/src/repository/service.test.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/auth/src/pages/cft-callback/index.test.ts
**/*.prisma
📄 CodeRabbit inference engine (CLAUDE.md)
Database tables and fields MUST be singular and snake_case (e.g.,
user,case,created_at). Use Prisma@@mapand@mapfor aliases.
Files:
apps/postgres/prisma/schema.prisma
**/{pages,locales}/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
Every page must support both English and Welsh by providing
enandcyobjects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Files:
libs/auth/src/pages/cft-callback/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/location-name-search/index.tslibs/auth/src/pages/cft-callback/index.test.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Page routes are created based on file names within the
pages/directory. Nested routes are created using subdirectories (e.g.,pages/admin/my-page.tsbecomes/admin/my-page).
Files:
libs/auth/src/pages/cft-callback/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/location-name-search/index.tslibs/auth/src/pages/cft-callback/index.test.ts
**/{locales,pages}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Files:
libs/auth/src/pages/cft-callback/index.tslibs/email-subscriptions/src/pages/delete-subscription/index.test.tslibs/email-subscriptions/src/pages/location-name-search/index.tslibs/auth/src/pages/cft-callback/index.test.ts
🧠 Learnings (4)
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{pages,locales}/**/*.{ts,njk} : Every page must support both English and Welsh by providing `en` and `cy` objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Applied to files:
libs/auth/src/middleware/navigation.test.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.
Applied to files:
libs/email-subscriptions/src/pages/delete-subscription/index.test.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{locales,pages}/**/*.ts : Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
🧬 Code graph analysis (8)
libs/auth/src/middleware/navigation.test.ts (1)
libs/auth/src/middleware/navigation.ts (1)
authNavigationMiddleware(9-35)
libs/email-subscriptions/src/repository/service.test.ts (1)
libs/email-subscriptions/src/repository/service.ts (5)
createSubscription(13-30)getSubscriptionsByUserId(32-34)removeSubscription(36-50)createMultipleSubscriptions(52-63)replaceUserSubscriptions(65-100)
libs/auth/src/pages/cft-callback/index.ts (2)
libs/account/src/repository/query.ts (1)
createOrUpdateUser(40-53)libs/auth/src/index.ts (1)
UserProfile(12-12)
libs/email-subscriptions/src/pages/delete-subscription/index.test.ts (1)
libs/email-subscriptions/src/pages/delete-subscription/index.ts (2)
GET(134-134)POST(135-135)
libs/auth/src/middleware/navigation.ts (1)
libs/auth/src/middleware/navigation-helper.ts (2)
buildVerifiedUserNavigation(67-99)buildNavigationItems(19-59)
libs/email-subscriptions/src/pages/location-name-search/index.ts (3)
libs/email-subscriptions/src/pages/location-name-search/cy.ts (1)
cy(1-24)libs/email-subscriptions/src/pages/location-name-search/en.ts (1)
en(1-24)libs/auth/src/middleware/navigation-helper.ts (1)
buildVerifiedUserNavigation(67-99)
libs/email-subscriptions/src/repository/service.ts (2)
libs/email-subscriptions/src/validation/validation.ts (1)
validateLocationId(4-7)libs/email-subscriptions/src/repository/queries.ts (5)
findSubscriptionByUserAndLocation(14-23)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)findSubscriptionsByUserId(3-12)deleteSubscriptionRecord(48-52)
e2e-tests/tests/email-subscriptions.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
⏰ 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 (31)
apps/postgres/prisma/schema.prisma (1)
38-38: Relation configuration is complete and properly implemented.All verification points pass:
- ✓ Subscription model is properly defined in
libs/email-subscriptions/prisma/schema.prismawith correct back-reference to User- ✓ All Subscription fields follow snake_case naming with @Map decorators (subscriptionId, userId, locationId, dateAdded)
- ✓ Migration creates the subscription table with proper foreign key constraint referencing user(user_id) with CASCADE delete/update
- ✓ User model primary key (userId @Map("user_id")) correctly matches the foreign key reference
- ✓ All naming conventions follow coding guidelines (singular table names, snake_case fields, @Map decorators)
The relation field added at line 38 (
subscriptions Subscription[]) is properly configured with complete constraint and index definitions.libs/email-subscriptions/src/pages/delete-subscription/index.test.ts (3)
1-14: LGTM!The imports correctly use
.jsextensions for relative imports, and the mocks are properly structured to isolate the delete-subscription handlers for testing.
41-107: LGTM!The GET handler tests comprehensively cover validation (missing/invalid UUID), authorization (subscription ownership), database errors, and success paths. The test structure is consistent and thorough.
109-164: Field naming is intentional; authorization and error handling already exist in POST implementation.The verification reveals the original review is partially incorrect:
Field naming inconsistency is intentional: Line 69-70 of the handler deliberately accepts both
subscriptionandsubscriptionIdfields via fallback logic (const subscriptionId = subscription || bodySubscriptionId;). The test usingsubscriptionIdat line 112 is valid.Authorization and error handling ARE implemented: The POST handler already includes:
- User ownership validation (lines 87-88:
if (!sub || sub.userId !== userId))- Database error handling (lines 90-93: catches errors during subscription lookup)
Both redirect to
/subscription-managementon failure, matching the test expectations.While the existing tests don't explicitly exercise all authorization and error paths for POST (they cover the happy paths), the suggested concern about "missing" functionality is unfounded—the implementation provides these safeguards.
libs/email-subscriptions/src/repository/service.test.ts (4)
1-26: LGTM! Well-structured test setup.The imports follow coding guidelines with
.jsextensions, and the mock setup withbeforeEach/afterEachcleanup is appropriate for isolating test cases.
28-77: LGTM! Comprehensive coverage of createSubscription paths.The test suite covers success, validation failures, duplicate detection, and subscription limits with appropriate mocks and assertions.
79-185: LGTM! Thorough test coverage for remaining service functions.All test suites demonstrate:
- Clear test organization and naming
- Proper mock configuration for each scenario
- Comprehensive edge case handling (not found, unauthorized, partial failures)
187-292: LGTM! Excellent test coverage for replaceUserSubscriptions.The test suite covers all critical scenarios:
- Adding and removing subscriptions simultaneously
- Add-only and remove-only operations
- No-op when lists match
- Max subscription enforcement
- Invalid location handling
libs/email-subscriptions/src/repository/service.ts (5)
1-11: LGTM! Clean imports and constant definition.The imports follow coding guidelines with
.jsextensions, and theMAX_SUBSCRIPTIONSconstant correctly usesSCREAMING_SNAKE_CASE.
13-30: LGTM! Well-structured validation flow.The function validates in a logical sequence (location validity → duplicate check → subscription limit) with clear error messages before creating the subscription.
32-34: LGTM! Simple and correct.This passthrough function appropriately delegates to the queries layer.
36-50: LGTM! Proper authorization checks.The function correctly validates both subscription existence and user ownership before allowing deletion.
52-63: LGTM! Appropriate batch operation handling.Using
Promise.allSettledis the right approach for batch operations where partial success is acceptable, and the return value properly reports both successes and failures.libs/auth/src/pages/cft-callback/index.test.ts (2)
1-1: LGTM!The shift to module-level mocking for
@hmcts/account/repository/queryis cleaner and aligns with Vitest best practices.Also applies to: 12-12
46-56: LGTM!The mocked user object correctly simulates the database response shape and aligns with the implementation's use of
dbUser.userIdfor session creation.libs/auth/src/middleware/navigation.test.ts (3)
205-238: LGTM!Test correctly validates English navigation for VERIFIED users, including the correct current state, href values, and data-test attributes.
240-273: LGTM!Test validates Welsh localization correctly, including the active state when the current path is
/subscription-management. This aligns with the bilingual support requirements.
275-296: LGTM!Test correctly validates the default locale fallback behavior, ensuring English is used when
res.locals.localeis not set.libs/auth/src/middleware/navigation.ts (1)
2-2: LGTM!Import correctly adds
buildVerifiedUserNavigationwith the required.jsextension for ESM compatibility.libs/auth/src/pages/cft-callback/index.ts (1)
49-63: LGTM!Critical improvement: The UserProfile now uses
dbUser.userId(Line 50) from the database response, ensuring session data is consistent with the persisted user record. This correctly sequences the database write before session creation.libs/email-subscriptions/src/pages/location-name-search/index.ts (4)
1-14: LGTM!Imports are correctly structured with workspace aliases and
.jsextensions for relative imports, following the coding guidelines.
16-56: LGTM!Query parameter normalization and filtering logic are well-structured. The
effectiveSubJurisdictionscomputation (Lines 38-45) correctly includes all sub-jurisdictions when jurisdictions are selected but no specific sub-jurisdictions are chosen, providing a good user experience.
58-145: LGTM!Display mapping and URL building logic are well-implemented. The cascading removal logic (Lines 118-122) correctly removes associated sub-jurisdictions when a jurisdiction is removed, preventing orphaned filters.
182-206: LGTM!POST handler correctly normalizes input, initializes session state, and persists data before redirecting. The authentication middleware (
requireAuth()andblockUserAccess()) is now properly included on both GET and POST exports.e2e-tests/tests/email-subscriptions.spec.ts (7)
1-28: LGTM!Test setup correctly authenticates before each test using environment variables and the CFT IDAM helper, ensuring a consistent authenticated state for all tests.
98-177: LGTM!Location Name Search Page tests are comprehensive, with appropriate waits for async content (e.g.,
waitForLoadState("networkidle")on Line 124) and explicit timeouts for reliability.
179-275: LGTM!Pending Subscriptions Page tests thoroughly validate required field validation, button presence, navigation flows, and accessibility with appropriate wait strategies.
277-354: LGTM!Subscription Confirmed Page tests correctly validate session-based access control and success confirmation flow, with appropriate timeouts for asynchronous redirects.
356-449: LGTM!Delete Subscription Page tests handle dynamic scenarios appropriately using conditional logic based on existing subscriptions (e.g., Lines 378-393), ensuring tests don't fail when no subscriptions exist.
451-529: LGTM!Unsubscribe Confirmation Page tests validate session-based access control and successful removal flow, with conditional logic for dynamic data scenarios.
531-618: LGTM!Authentication protection tests comprehensively validate that all subscription pages require authentication. The complete flow test provides excellent end-to-end coverage of the entire subscription journey. Note that Line 583 also uses the
.nth(2)selector pattern mentioned earlier.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
e2e-tests/tests/email-subscriptions.spec.ts (4)
20-24: Add validation for required environment variables.The non-null assertions assume
CFT_VALID_TEST_ACCOUNTandCFT_VALID_TEST_ACCOUNT_PASSWORDare defined. Consider adding validation at the test suite level to provide clear error messages if these environment variables are missing.Apply this diff to add validation:
test.describe("Email Subscriptions", () => { + test.beforeAll(() => { + if (!process.env.CFT_VALID_TEST_ACCOUNT || !process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD) { + throw new Error( + "Required environment variables are missing: CFT_VALID_TEST_ACCOUNT and CFT_VALID_TEST_ACCOUNT_PASSWORD" + ); + } + }); + // Authenticate before each test test.beforeEach(async ({ page }) => {
47-48: Replace positional selector with semantic or data-testid attribute.Using
.verified-tile.nth(2)is brittle—if tiles are reordered or new tiles are added, the test will break. Consider using a data-testid attribute or a more specific semantic selector (e.g., a link with text matching "email subscriptions").Example with data-testid:
- const emailSubsTile = page.locator(".verified-tile").nth(2); + const emailSubsTile = page.locator('[data-testid="email-subscriptions-tile"]'); await emailSubsTile.click();Or with semantic selector:
- const emailSubsTile = page.locator(".verified-tile").nth(2); + const emailSubsTile = page.getByRole("link", { name: /email subscriptions/i }); await emailSubsTile.click();Also applies to: 593-594
366-377: Replace hardcoded timeout with explicit wait conditions.The hardcoded
page.waitForTimeout(1000)is an anti-pattern that can make tests flaky. Use Playwright's built-in wait mechanisms likewaitForURL()orwaitForLoadState()instead.Apply this diff:
test("should redirect for invalid subscription ID", async ({ page }) => { await page.goto("/delete-subscription?subscriptionId=invalid-id"); - // Should redirect or show error - await page.waitForTimeout(1000); - - // Either shows 400 error or redirects to subscription management - const is400Error = await page.locator("text=400").isVisible().catch(() => false); - const isSubManagement = page.url().includes("/subscription-management"); - - expect(is400Error || isSubManagement).toBeTruthy(); + // Should redirect to subscription management or show error page + await Promise.race([ + page.waitForURL(/\/subscription-management/), + page.waitForSelector("text=400"), + ]); + + const currentUrl = page.url(); + const has400Error = await page.locator("text=400").isVisible().catch(() => false); + + expect(has400Error || currentUrl.includes("/subscription-management")).toBeTruthy(); });
542-549: Update misleading comment.The comment states "Create new context without authentication", but the code only clears cookies from the existing context.
Apply this diff:
test("should require authentication for subscription management", async ({ page, context }) => { - // Create new context without authentication + // Clear authentication cookies await context.clearCookies(); await page.goto("/subscription-management");libs/email-subscriptions/src/pages/location-name-search/index.njk (1)
27-35: Consider moving inline styles to CSS classes.Multiple inline styles are used throughout the template (lines 27, 30, 35, 89, 163). Consider consolidating these into reusable CSS classes to improve maintainability and consistency.
libs/email-subscriptions/src/pages/location-name-search/index.ts (3)
178-179: Avoid type assertion toanyfor CSRF token.Consider extending the Express Request type or using a type guard instead of casting to
any.- csrfToken: (req as any).csrfToken?.() || "" + csrfToken: typeof req.csrfToken === "function" ? req.csrfToken() : ""Alternatively, extend the Request type in a declaration file to include
csrfToken.
188-196: Consider validating locationIds input.The
locationIdsfromreq.bodyare stored directly in the session without validation. Consider sanitizing to ensure they are valid location ID strings/numbers before persisting.// Handle both single and multiple selections -const selectedLocationIds = Array.isArray(locationIds) ? locationIds : locationIds ? [locationIds] : []; +const selectedLocationIds = (Array.isArray(locationIds) ? locationIds : locationIds ? [locationIds] : []) + .map(String) + .filter((id: string) => /^\d+$/.test(id));
199-206: Session save error handling redirects back without user feedback.When session save fails, the user is redirected back to the search page without any indication of the error. Consider adding flash message support or rendering an error state.
📜 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 (3)
e2e-tests/tests/email-subscriptions.spec.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.njk(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/{pages,locales}/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
Every page must support both English and Welsh by providing
enandcyobjects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Files:
libs/email-subscriptions/src/pages/location-name-search/index.njklibs/email-subscriptions/src/pages/location-name-search/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/email-subscriptions/src/pages/location-name-search/index.tse2e-tests/tests/email-subscriptions.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/email-subscriptions/src/pages/location-name-search/index.tse2e-tests/tests/email-subscriptions.spec.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Page routes are created based on file names within the
pages/directory. Nested routes are created using subdirectories (e.g.,pages/admin/my-page.tsbecomes/admin/my-page).
Files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
**/{locales,pages}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.njk : Nunjucks templates must extend `layouts/default.njk` and use GOV.UK Design System macros. Every page must support both English and Welsh content.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.njk
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
🧬 Code graph analysis (1)
e2e-tests/tests/email-subscriptions.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
⏰ 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 (5)
e2e-tests/tests/email-subscriptions.spec.ts (2)
1-3: LGTM!Imports are correctly structured with the required
.jsextension for relative imports as per coding guidelines.
79-87: Verify intentional disabling of accessibility rule.The "region" accessibility rule is disabled across all Axe tests. Ensure this is intentional and that the pages either don't require landmark regions or have a documented exception.
The "region" rule ensures proper landmark regions (header, main, footer, etc.) for screen readers. If this is a known limitation, consider adding a comment explaining why it's disabled.
libs/email-subscriptions/src/pages/location-name-search/index.njk (2)
1-8: Template structure follows GOV.UK patterns.The template correctly extends the base layout, imports GOV.UK Design System macros, and includes proper page title structure with service name. CSRF protection is properly implemented.
146-153: The review comment is based on an incorrect premise about the data model.The
Locationinterface defineswelshNameas a requiredstringproperty (not optional). All location objects inlibs/location/src/location-data.tsare statically defined withwelshNamepopulated. TypeScript's type system guaranteeswelshNamewill never benullorundefinedat runtime—the template code is type-safe and requires no fallback handling.This pattern is consistent across the entire codebase (all pages follow
locale === "cy" ? location.welshName : location.namewithout fallbacks). The page also correctly maintains bilingual support with matchingen.tsandcy.tslocale files.Likely an incorrect or invalid review comment.
libs/email-subscriptions/src/pages/location-name-search/index.ts (1)
208-209: Auth middleware properly applied.The route handlers now correctly include
requireAuth()andblockUserAccess()middleware, addressing the previous review concern about protecting subscription-flow routes.
libs/email-subscriptions/src/pages/location-name-search/index.ts
Outdated
Show resolved
Hide resolved
libs/email-subscriptions/src/pages/location-name-search/index.ts
Outdated
Show resolved
Hide resolved
libs/email-subscriptions/src/pages/location-name-search/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e-tests/tests/email-subscriptions.spec.ts (1)
379-457: Conditionalif (count > 0)blocks let delete/unsubscribe tests silently do nothingThe delete and unsubscribe tests all guard their core interactions/assertions with
if (count > 0). When there are no subscriptions,countis 0 and the tests effectively become no-ops but still pass, so they don’t actually validate the delete/unsubscribe flows.This was raised in the earlier review and still applies here. To make these tests reliable:
- Ensure test data is created deterministically (e.g. in a
test.beforeEachwithin the “Delete Subscription Page” and “Unsubscribe Confirmation Page”describeblocks) by driving the UI to add at least one subscription, then- Remove the
if (count > 0)branches so each test always exercises the flow and always runs its assertions.For example, within the delete suite:
test.describe("Delete Subscription Page", () => { + test.beforeEach(async ({ page }) => { + // Ensure at least one subscription exists + await page.goto("/location-name-search"); + await page.waitForLoadState("networkidle"); + const postForm = page.locator("form[method='post']"); + const firstCheckbox = postForm.locator("input[type='checkbox']").first(); + await firstCheckbox.waitFor({ state: "visible" }); + await firstCheckbox.check(); + await postForm.getByRole("button", { name: /continue/i }).click(); + await page.getByRole("button", { name: /confirm/i }).click(); + await page.goto("/subscription-management"); + }); @@ - const deleteLinks = page.getByRole("button", { name: /remove/i }); - const count = await deleteLinks.count(); - - if (count > 0) { - await deleteLinks.first().click(); - // ... - } + const deleteLinks = page.getByRole("button", { name: /remove/i }); + await deleteLinks.first().click(); + // ...You can apply the same pattern to the unsubscribe confirmation tests so they always create, then remove, a subscription.
This will turn these from “best-effort” tests into deterministic checks that will reliably catch regressions in deletion/unsubscribe flows.
Also applies to: 468-538
🧹 Nitpick comments (2)
e2e-tests/tests/email-subscriptions.spec.ts (2)
47-48: Avoid relying on.verified-tile.nth(2) for navigation assertionsUsing
.verified-tile.nth(2) tightly couples these tests to the current DOM ordering of tiles. Any reordering or addition of tiles will break both the “navigation to subscription management” test and the “complete subscription flow” test even though the user-visible behaviour is unchanged.Consider introducing a stable attribute on the email-subscriptions tile (e.g.
data-testid="email-subscriptions-tile") and targeting that instead, so the locator is resilient to layout changes:- const emailSubsTile = page.locator(".verified-tile").nth(2); + const emailSubsTile = page.locator("[data-testid='email-subscriptions-tile']");Same applies to the usage in the “Complete Subscription Flow” block.
Also applies to: 593-595
365-377: Make invalid subscription ID behaviour deterministic and avoidwaitForTimeoutThis test currently:
- Sleeps for a fixed 1s via
waitForTimeout, and- Accepts either a 400 error or a redirect to
/subscription-management.That combination makes the test both slower and less strict: regressions that change timing or behaviour but still match one branch of the OR will go unnoticed.
If the product behaviour can be pinned down, it would be better to:
- Assert a single expected outcome (either always 400 or always redirect), and
- Wait on a specific condition (e.g.
expect(page).toHaveURL(...)orexpect(errorLocator).toBeVisible()) instead of a fixed timeout.This will make the test faster and failures easier to diagnose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/tests/email-subscriptions.spec.ts(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/email-subscriptions/src/pages/location-name-search/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
e2e-tests/tests/email-subscriptions.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{pages,locales}/**/*.{ts,njk} : Every page must support both English and Welsh by providing `en` and `cy` objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Applied to files:
e2e-tests/tests/email-subscriptions.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/email-subscriptions.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/assets/css/index.scss(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.njk(1 hunks)libs/web-core/src/assets/css/button-as-link.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/email-subscriptions/src/pages/subscription-management/index.njk
🧰 Additional context used
🪛 GitHub Actions: E2E Tests
apps/web/src/assets/css/index.scss
[error] 7-7: Sass error: Missing "./src/assets/css/button-as-link.scss" specifier in "@hmcts/web-core" package.
⏰ 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). (1)
- GitHub Check: SonarQube Analysis
🔇 Additional comments (1)
libs/web-core/src/assets/css/button-as-link.scss (1)
1-30: Well-structured accessible button-as-link styling.The
.govuk-button-as-linkimplementation is solid. It properly resets button styling, inherits typography, follows GOV.UK colour conventions (#1d70b8 for default, #003078 for hover, #0b0c0c for active), and implements an accessible focus state with yellow highlight and shadow.The focus state correctly uses the standard GOV.UK pattern of a transparent outline combined with a box-shadow, ensuring keyboard users have clear visual feedback. Once the import path in
apps/web/src/assets/css/index.scssis resolved, this class will be ready for use in verified user subscription flows.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/postgres/src/schema-discovery.test.ts (1)
15-19: Test correctly reflects new behavior; consider asserting full array structure (optional).This test now accurately matches
getPrismaSchemas()returning[emailSubscriptionsSchemas, locationSchemas]. If you want the expectation to be a bit more declarative (and cover length + order in one assertion), you could write:const result = getPrismaSchemas(); expect(result).toEqual([ expect.stringContaining("email-subscriptions"), expect.stringContaining("location"), ]);This keeps the intent clear while still locking in both entries and their order.
📜 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 (5)
apps/postgres/src/schema-discovery.test.ts(1 hunks)apps/postgres/src/schema-discovery.ts(1 hunks)apps/web/src/app.test.ts(1 hunks)apps/web/src/app.ts(3 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tsconfig.json
- apps/postgres/src/schema-discovery.ts
- apps/web/src/app.test.ts
- apps/web/src/app.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 should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
apps/postgres/src/schema-discovery.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
apps/postgres/src/schema-discovery.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All packages must use
"test": "vitest run"as the test script. Unit and integration tests must be co-located with source code as*.test.tsfiles.
Files:
apps/postgres/src/schema-discovery.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 116
File: apps/postgres/prisma/migrations/20251121153203_create_user/migration.sql:1-17
Timestamp: 2025-11-21T17:06:40.430Z
Learning: In the cath-service repository, email is always populated by the SSO and CFT authentication flows, so the NOT NULL constraint on the email column in the user table is safe.
🧬 Code graph analysis (1)
apps/postgres/src/schema-discovery.test.ts (1)
apps/postgres/src/schema-discovery.ts (1)
getPrismaSchemas(5-7)
⏰ 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). (1)
- GitHub Check: SonarQube Analysis
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/email-subscriptions/src/pages/location-name-search/index.ts (1)
193-193: Type the CSRF token access properly.The code uses
(req as any).csrfToken?.()which bypasses TypeScript's type safety. Consider extending the Express Request interface to include thecsrfTokenmethod added by the CSRF middleware.Create a types file (e.g.,
libs/email-subscriptions/src/types/express.d.ts):import 'express'; declare module 'express' { interface Request { csrfToken?: () => string; } }Then use it directly:
- csrfToken: (req as any).csrfToken?.() || "" + csrfToken: req.csrfToken?.() || ""
📜 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 (5)
libs/email-subscriptions/package.json(1 hunks)libs/email-subscriptions/src/pages/location-name-search/index.ts(1 hunks)libs/email-subscriptions/src/pages/pending-subscriptions/index.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-confirmed/index.ts(1 hunks)libs/email-subscriptions/src/pages/subscription-management/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/email-subscriptions/package.json
- libs/email-subscriptions/src/pages/pending-subscriptions/index.ts
- libs/email-subscriptions/src/pages/subscription-management/index.ts
🧰 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 should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.ts
**/{pages,locales}/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
Every page must support both English and Welsh by providing
enandcyobjects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Files:
libs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Page routes are created based on file names within the
pages/directory. Nested routes are created using subdirectories (e.g.,pages/admin/my-page.tsbecomes/admin/my-page).
Files:
libs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.ts
**/{locales,pages}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Files:
libs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.ts
🧠 Learnings (5)
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.tslibs/email-subscriptions/src/pages/subscription-confirmed/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/**/*.ts : Page routes are created based on file names within the `pages/` directory. Nested routes are created using subdirectories (e.g., `pages/admin/my-page.ts` becomes `/admin/my-page`).
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{locales,pages}/**/*.ts : Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
📚 Learning: 2025-11-25T06:40:24.082Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : TypeScript must use strict mode enabled with no `any` without justification. Use workspace aliases (`hmcts/*`) for imports.
Applied to files:
libs/email-subscriptions/src/pages/location-name-search/index.ts
🧬 Code graph analysis (1)
libs/email-subscriptions/src/pages/subscription-confirmed/index.ts (3)
libs/email-subscriptions/src/pages/subscription-confirmed/cy.ts (1)
cy(1-11)libs/email-subscriptions/src/pages/subscription-confirmed/en.ts (1)
en(1-11)libs/auth/src/middleware/navigation-helper.ts (1)
buildVerifiedUserNavigation(67-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 (3)
libs/email-subscriptions/src/pages/location-name-search/index.ts (3)
1-15: LGTM!Imports follow all coding guidelines: workspace aliases are used correctly, relative imports include the
.jsextension as required for ESM, and organization is clear.
17-21: LGTM!The
TableRowinterface is well-defined with proper typing, addressing the previous concern about usinganytypes for table data structures.
220-221: LGTM! Auth middleware properly restored.The GET and POST handlers are now correctly protected with
requireAuth()andblockUserAccess()middleware, addressing the previous critical security concern about unprotected subscription routes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
turbo.json(1 hunks)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
libs/verified-pages/src/pages/location-name-search/index.ts (1)
197-208: ValidatelocationIdsinput before storing in session.The POST handler stores
locationIdsfrom the request body without validating they are numeric/valid. This is inconsistent with the GET handler's thorough validation (lines 31-47) and violates the coding guideline that all input endpoints must validate inputs.Apply validation consistent with GET handler:
const postHandler = async (req: Request, res: Response) => { const locationIds = req.body.locationIds; // Handle both single and multiple selections - const selectedLocationIds = Array.isArray(locationIds) ? locationIds : locationIds ? [locationIds] : []; + const rawIds = Array.isArray(locationIds) ? locationIds : locationIds ? [locationIds] : []; + const selectedLocationIds = rawIds + .map((id: string) => Number(id)) + .filter(Number.isFinite) + .map(String); if (!req.session.emailSubscriptions) {
🧹 Nitpick comments (11)
libs/verified-pages/src/pages/location-name-search/en.ts (1)
1-24: LGTM! Structure matches Welsh counterpart.The locale object is well-organized with all necessary UI strings. Minor note:
backandcontinueButtonalso exist inlibs/subscriptions/src/locales/en.ts. Consider importing shared content from module-level locales to reduce duplication, per coding guidelines. This is optional if page-specific wording flexibility is preferred.libs/verified-pages/src/pages/location-name-search/index.test.ts (2)
46-76: GET tests cover filter scenarios well.Consider adding a test for Welsh locale rendering to verify the
cycontent is used whenres.locals.locale = "cy". This would ensure bilingual support is exercised in tests.it("should render page with Welsh locale", async () => { mockRes.locals = { locale: "cy" }; await GET[GET.length - 1](mockReq as Request, mockRes as Response, vi.fn()); expect(mockRes.render).toHaveBeenCalledWith( "location-name-search/index", expect.objectContaining({ locale: "cy" }) ); });
78-114: POST tests cover main selection flows.Consider adding a test for the session save error scenario to verify the error handling path redirects back to
/location-name-search:it("should redirect back on session save error", async () => { mockReq.body = { locationIds: "123" }; mockReq.session = { save: vi.fn((callback: (err: Error | null) => void) => callback(new Error("Save failed"))) } as any; await POST[POST.length - 1](mockReq as Request, mockRes as Response, vi.fn()); expect(mockRes.redirect).toHaveBeenCalledWith("/location-name-search"); });libs/verified-pages/src/pages/location-name-search/index.njk (2)
194-198: Improve back-to-top link accessibility.The
href="#"is not ideal for accessibility and doesn't provide meaningful navigation. Consider linking to a top anchor or usinghref="javascript:void(0)"with proper event handling.- <a href="#" class="govuk-link back-to-top-link"><span aria-hidden="true">▴ </span>{{ backToTop }}</a> + <a href="#main-content" class="govuk-link back-to-top-link"><span aria-hidden="true">▴ </span>{{ backToTop }}</a>Alternatively, add
id="main-content"to the page heading or a top-level container.
27-35: Consider moving inline styles to CSS.Multiple inline styles (
style="border: 1px solid...",style="background-color:...") reduce maintainability. Consider extracting these to CSS classes in the project's stylesheet for consistency and easier theming.libs/verified-pages/src/pages/location-name-search/index.ts (2)
17-21: Remove unusedisFirstproperty from TableRow interface.The
isFirstproperty is computed at line 171 but never used in the template. The template usesletterbeing empty/non-empty to determine first row rendering.interface TableRow { letter: string; location: Location; - isFirst: boolean; }And update line 171:
- isFirst: index === 0
193-194: Consider type augmentation instead ofanyassertion.The
(req as any).csrfToken?.()bypasses type safety. Per coding guidelines, avoidanywithout justification. Consider augmenting the Express Request type:// In a types/express.d.ts file declare global { namespace Express { interface Request { csrfToken?: () => string; } } }Then use
req.csrfToken?.() || ""without the assertion.apps/postgres/src/schema-discovery.test.ts (1)
15-20: Consider order-independent assertions.The test currently assumes a specific array order for schema paths. If the schema order changes in the implementation, the test will fail even though the functionality remains correct.
Consider refactoring to check for presence without assuming order:
- it("should return array with subscriptions and location schemas", () => { + it("should return array with subscriptions and location schemas", () => { const result = getPrismaSchemas(); expect(result.length).toBe(2); - expect(result[0]).toContain("subscriptions"); - expect(result[1]).toContain("location"); + expect(result.some(path => path.includes("subscriptions"))).toBe(true); + expect(result.some(path => path.includes("location"))).toBe(true); });libs/verified-pages/src/pages/delete-subscription/index.test.ts (1)
19-19: Restore the console.error spy after tests.The
consoleErrorSpyis created at the describe block level but never restored. This can leak mocked behavior to other test suites running in the same process.afterEach(() => { vi.clearAllMocks(); }); + + afterAll(() => { + consoleErrorSpy.mockRestore(); + });libs/verified-pages/src/pages/subscription-confirmed/index.ts (1)
15-24: Add error handling for location lookups to prevent page crashes.If
getLocationByIdthrows (e.g., database connectivity issue),Promise.allwill reject and crash the page with a 500 error. Consider wrapping each lookup in a try/catch to gracefully degrade.const confirmedLocations = ( await Promise.all( confirmedLocationIds.map(async (id: string) => { - const location = await getLocationById(Number.parseInt(id, 10)); - return location ? (locale === "cy" ? location.welshName : location.name) : null; + try { + const location = await getLocationById(Number.parseInt(id, 10)); + return location ? (locale === "cy" ? location.welshName : location.name) : null; + } catch { + return null; + } }) ) ).filter(Boolean);libs/subscriptions/src/repository/service.ts (1)
36-50: Use the existingfindSubscriptionByIdquery helper for consistency.
removeSubscriptiondirectly usesprisma.subscription.findUniquewhile the same query is available viafindSubscriptionByIdinqueries.ts. This duplicates logic and breaks the pattern used elsewhere in this service.+import { + countSubscriptionsByUserId, + createSubscriptionRecord, + deleteSubscriptionRecord, + findSubscriptionById, + findSubscriptionByUserAndLocation, + findSubscriptionsByUserId +} from "./queries.js"; -import { prisma } from "@hmcts/postgres"; export async function removeSubscription(subscriptionId: string, userId: string) { - const subscription = await prisma.subscription.findUnique({ - where: { subscriptionId } - }); + const subscription = await findSubscriptionById(subscriptionId); if (!subscription) { throw new Error("Subscription not found");
📜 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 (47)
apps/postgres/src/schema-discovery.test.ts(1 hunks)apps/postgres/src/schema-discovery.ts(1 hunks)libs/subscriptions/package.json(1 hunks)libs/subscriptions/prisma/schema.prisma(1 hunks)libs/subscriptions/src/config.ts(1 hunks)libs/subscriptions/src/index.ts(1 hunks)libs/subscriptions/src/locales/cy.ts(1 hunks)libs/subscriptions/src/locales/en.ts(1 hunks)libs/subscriptions/src/repository/queries.test.ts(1 hunks)libs/subscriptions/src/repository/queries.ts(1 hunks)libs/subscriptions/src/repository/service.test.ts(1 hunks)libs/subscriptions/src/repository/service.ts(1 hunks)libs/subscriptions/src/validation/validation.test.ts(1 hunks)libs/subscriptions/src/validation/validation.ts(1 hunks)libs/subscriptions/tsconfig.json(1 hunks)libs/verified-pages/package.json(1 hunks)libs/verified-pages/src/pages/delete-subscription/cy.ts(1 hunks)libs/verified-pages/src/pages/delete-subscription/en.ts(1 hunks)libs/verified-pages/src/pages/delete-subscription/index.njk(1 hunks)libs/verified-pages/src/pages/delete-subscription/index.test.ts(1 hunks)libs/verified-pages/src/pages/delete-subscription/index.ts(1 hunks)libs/verified-pages/src/pages/location-name-search/cy.ts(1 hunks)libs/verified-pages/src/pages/location-name-search/en.ts(1 hunks)libs/verified-pages/src/pages/location-name-search/index.njk(1 hunks)libs/verified-pages/src/pages/location-name-search/index.test.ts(1 hunks)libs/verified-pages/src/pages/location-name-search/index.ts(1 hunks)libs/verified-pages/src/pages/pending-subscriptions/cy.ts(1 hunks)libs/verified-pages/src/pages/pending-subscriptions/en.ts(1 hunks)libs/verified-pages/src/pages/pending-subscriptions/index.njk(1 hunks)libs/verified-pages/src/pages/pending-subscriptions/index.test.ts(1 hunks)libs/verified-pages/src/pages/pending-subscriptions/index.ts(1 hunks)libs/verified-pages/src/pages/subscription-confirmed/cy.ts(1 hunks)libs/verified-pages/src/pages/subscription-confirmed/en.ts(1 hunks)libs/verified-pages/src/pages/subscription-confirmed/index.njk(1 hunks)libs/verified-pages/src/pages/subscription-confirmed/index.test.ts(1 hunks)libs/verified-pages/src/pages/subscription-confirmed/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/verified-pages/src/pages/subscription-management/index.test.ts(1 hunks)libs/verified-pages/src/pages/subscription-management/index.ts(1 hunks)libs/verified-pages/src/pages/unsubscribe-confirmation/cy.ts(1 hunks)libs/verified-pages/src/pages/unsubscribe-confirmation/en.ts(1 hunks)libs/verified-pages/src/pages/unsubscribe-confirmation/index.njk(1 hunks)libs/verified-pages/src/pages/unsubscribe-confirmation/index.test.ts(1 hunks)libs/verified-pages/src/pages/unsubscribe-confirmation/index.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/postgres/src/schema-discovery.ts
- tsconfig.json
🧰 Additional context used
📓 Path-based instructions (10)
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
All package.json files must use"type": "module"to enforce ES modules. Never use CommonJSrequire()ormodule.exports. Useimport/exportonly.
Express version 5.x only must be used ("express": "5.1.0"). Pin all dependencies to specific versions except peer dependencies.
Build scripts must includebuild:nunjucksif the module contains Nunjucks templates in thepages/directory to copy .njk files to dist.
Files:
libs/verified-pages/package.jsonlibs/subscriptions/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/verified-pages/src/pages/location-name-search/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.test.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/subscriptions/src/locales/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.tslibs/subscriptions/src/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/subscriptions/src/repository/queries.tslibs/subscriptions/src/validation/validation.tsapps/postgres/src/schema-discovery.test.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/location-name-search/index.tslibs/verified-pages/src/pages/subscription-confirmed/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/subscriptions/src/repository/queries.test.tslibs/subscriptions/src/validation/validation.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/unsubscribe-confirmation/en.tslibs/verified-pages/src/pages/location-name-search/index.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/locales/cy.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/pages/delete-subscription/en.tslibs/subscriptions/src/config.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/verified-pages/src/pages/location-name-search/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.test.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/subscriptions/src/locales/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.tslibs/subscriptions/src/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/subscriptions/src/repository/queries.tslibs/subscriptions/src/validation/validation.tsapps/postgres/src/schema-discovery.test.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/subscriptions/src/repository/service.tslibs/verified-pages/src/pages/location-name-search/index.tslibs/verified-pages/src/pages/subscription-confirmed/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/subscriptions/src/repository/queries.test.tslibs/subscriptions/src/validation/validation.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/unsubscribe-confirmation/en.tslibs/verified-pages/src/pages/location-name-search/index.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/locales/cy.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/pages/delete-subscription/en.tslibs/subscriptions/src/config.ts
**/{pages,locales}/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
Every page must support both English and Welsh by providing
enandcyobjects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Files:
libs/verified-pages/src/pages/location-name-search/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.test.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/subscriptions/src/locales/en.tslibs/verified-pages/src/pages/location-name-search/index.njklibs/verified-pages/src/pages/subscription-confirmed/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/pending-subscriptions/index.njklibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/verified-pages/src/pages/location-name-search/index.tslibs/verified-pages/src/pages/subscription-confirmed/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/verified-pages/src/pages/subscription-management/index.njklibs/verified-pages/src/pages/unsubscribe-confirmation/index.njklibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/verified-pages/src/pages/delete-subscription/index.njklibs/verified-pages/src/pages/subscription-confirmed/index.njklibs/verified-pages/src/pages/unsubscribe-confirmation/en.tslibs/verified-pages/src/pages/location-name-search/index.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/locales/cy.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/pages/delete-subscription/en.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Page routes are created based on file names within the
pages/directory. Nested routes are created using subdirectories (e.g.,pages/admin/my-page.tsbecomes/admin/my-page).
Files:
libs/verified-pages/src/pages/location-name-search/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.test.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/verified-pages/src/pages/subscription-confirmed/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/verified-pages/src/pages/location-name-search/index.tslibs/verified-pages/src/pages/subscription-confirmed/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/en.tslibs/verified-pages/src/pages/location-name-search/index.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/pages/delete-subscription/en.ts
**/{locales,pages}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Files:
libs/verified-pages/src/pages/location-name-search/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.test.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/subscriptions/src/locales/en.tslibs/verified-pages/src/pages/subscription-confirmed/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/verified-pages/src/pages/location-name-search/index.tslibs/verified-pages/src/pages/subscription-confirmed/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/en.tslibs/verified-pages/src/pages/location-name-search/index.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/subscriptions/src/locales/cy.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/verified-pages/src/pages/delete-subscription/en.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All packages must use
"test": "vitest run"as the test script. Unit and integration tests must be co-located with source code as*.test.tsfiles.
Files:
libs/verified-pages/src/pages/subscription-confirmed/index.test.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tsapps/postgres/src/schema-discovery.test.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/subscriptions/src/repository/queries.test.tslibs/subscriptions/src/validation/validation.test.tslibs/subscriptions/src/repository/service.test.tslibs/verified-pages/src/pages/location-name-search/index.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.ts
libs/*/src/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All modules must have
src/index.tsfor business logic exports separate fromsrc/config.ts.
Files:
libs/subscriptions/src/index.ts
**/*.prisma
📄 CodeRabbit inference engine (CLAUDE.md)
Database tables and fields MUST be singular and snake_case (e.g.,
user,case,created_at). Use Prisma@@mapand@mapfor aliases.
Files:
libs/subscriptions/prisma/schema.prisma
**/config.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate
config.tsfile to avoid circular dependencies during Prisma client generation. Apps must import config using the/configpath (e.g.,@hmcts/my-feature/config).
Files:
libs/subscriptions/src/config.ts
🧠 Learnings (15)
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/package.json : Package names must use hmcts scope (e.g., `hmcts/auth`, `hmcts/case-management`).
Applied to files:
libs/verified-pages/package.jsonlibs/subscriptions/package.json
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/package.json : Express version 5.x only must be used (`"express": "5.1.0"`). Pin all dependencies to specific versions except peer dependencies.
Applied to files:
libs/verified-pages/package.json
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/{locales,pages}/**/*.ts : Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Applied to files:
libs/verified-pages/src/pages/location-name-search/en.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/subscriptions/src/locales/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/verified-pages/src/pages/location-name-search/index.tslibs/verified-pages/src/pages/subscription-confirmed/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/en.tslibs/subscriptions/src/locales/cy.tslibs/verified-pages/src/pages/delete-subscription/en.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/{pages,locales}/**/*.{ts,njk} : Every page must support both English and Welsh by providing `en` and `cy` objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Applied to files:
libs/verified-pages/src/pages/location-name-search/en.tslibs/verified-pages/src/pages/subscription-management/en.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/subscriptions/src/locales/en.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/verified-pages/src/pages/subscription-confirmed/en.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/subscriptions/src/locales/cy.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.
Applied to files:
libs/verified-pages/src/pages/subscription-confirmed/index.test.tslibs/subscriptions/src/validation/validation.test.tslibs/verified-pages/src/pages/location-name-search/index.test.tslibs/subscriptions/tsconfig.jsonlibs/verified-pages/src/pages/subscription-management/index.test.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.
Applied to files:
libs/verified-pages/src/pages/subscription-management/index.tslibs/verified-pages/src/pages/delete-subscription/cy.tslibs/verified-pages/src/pages/subscription-confirmed/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/verified-pages/src/pages/location-name-search/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/delete-subscription/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/verified-pages/src/pages/pending-subscriptions/en.tslibs/verified-pages/src/pages/location-name-search/index.test.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/delete-subscription/cy.tslibs/verified-pages/src/pages/unsubscribe-confirmation/cy.tslibs/verified-pages/src/pages/location-name-search/cy.tslibs/verified-pages/src/pages/subscription-confirmed/cy.tslibs/verified-pages/src/pages/subscription-management/cy.tslibs/verified-pages/src/pages/pending-subscriptions/cy.tslibs/subscriptions/src/locales/cy.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/pages/*.njk : Nunjucks templates must extend `layouts/default.njk` and use GOV.UK Design System macros. Every page must support both English and Welsh content.
Applied to files:
libs/verified-pages/src/pages/location-name-search/index.njklibs/verified-pages/src/pages/pending-subscriptions/index.njklibs/verified-pages/src/pages/subscription-management/index.njklibs/verified-pages/src/pages/unsubscribe-confirmation/index.njklibs/verified-pages/src/pages/delete-subscription/index.njklibs/verified-pages/src/pages/subscription-confirmed/index.njk
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to libs/*/src/index.ts : All modules must have `src/index.ts` for business logic exports separate from `src/config.ts`.
Applied to files:
libs/subscriptions/src/index.tslibs/subscriptions/package.jsonlibs/subscriptions/tsconfig.jsonlibs/subscriptions/src/config.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Applied to files:
libs/subscriptions/src/index.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/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 must import config using the `/config` path (e.g., `hmcts/my-feature/config`).
Applied to files:
libs/subscriptions/src/index.tslibs/subscriptions/src/config.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/package.json : All package.json files must use `"type": "module"` to enforce ES modules. Never use CommonJS `require()` or `module.exports`. Use `import`/`export` only.
Applied to files:
libs/subscriptions/package.json
📚 Learning: 2025-11-27T14:18:22.922Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 137
File: e2e-tests/tests/create-media-account.spec.ts:51-64
Timestamp: 2025-11-27T14:18:22.922Z
Learning: For the create-media-account form in libs/public-pages, the English email validation error message (errorEmailInvalid) should be: "There is a problem - Enter a valid email address, e.g. nameexample.com" to match the Welsh translation and clearly indicate the format requirement rather than suggesting the field is empty.
Applied to files:
libs/verified-pages/src/pages/subscription-management/cy.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Always add `.js` extension to relative imports (e.g., `import { foo } from "./bar.js"`). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Applied to files:
libs/subscriptions/tsconfig.jsonlibs/subscriptions/src/config.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
Applied to files:
libs/subscriptions/tsconfig.json
🧬 Code graph analysis (25)
libs/verified-pages/src/pages/location-name-search/en.ts (6)
libs/subscriptions/src/locales/en.ts (1)
en(1-7)libs/verified-pages/src/pages/delete-subscription/en.ts (1)
en(1-9)libs/verified-pages/src/pages/pending-subscriptions/en.ts (1)
en(1-12)libs/verified-pages/src/pages/subscription-confirmed/en.ts (1)
en(1-11)libs/verified-pages/src/pages/subscription-management/en.ts (1)
en(1-10)libs/verified-pages/src/pages/unsubscribe-confirmation/en.ts (1)
en(1-11)
libs/verified-pages/src/pages/subscription-confirmed/index.test.ts (1)
libs/verified-pages/src/pages/subscription-confirmed/index.ts (1)
GET(44-44)
libs/verified-pages/src/pages/subscription-management/index.ts (3)
libs/verified-pages/src/pages/subscription-management/cy.ts (1)
cy(1-10)libs/verified-pages/src/pages/subscription-management/en.ts (1)
en(1-10)libs/subscriptions/src/repository/service.ts (1)
getSubscriptionsByUserId(32-34)
libs/verified-pages/src/pages/delete-subscription/cy.ts (6)
libs/subscriptions/src/locales/cy.ts (1)
cy(1-7)libs/verified-pages/src/pages/location-name-search/cy.ts (1)
cy(1-24)libs/verified-pages/src/pages/pending-subscriptions/cy.ts (1)
cy(1-12)libs/verified-pages/src/pages/subscription-confirmed/cy.ts (1)
cy(1-11)libs/verified-pages/src/pages/subscription-management/cy.ts (1)
cy(1-10)libs/verified-pages/src/pages/unsubscribe-confirmation/cy.ts (1)
cy(1-11)
libs/subscriptions/src/locales/en.ts (2)
libs/verified-pages/src/pages/location-name-search/en.ts (1)
en(1-24)libs/verified-pages/src/pages/pending-subscriptions/en.ts (1)
en(1-12)
libs/verified-pages/src/pages/subscription-confirmed/index.ts (2)
libs/verified-pages/src/pages/subscription-confirmed/cy.ts (1)
cy(1-11)libs/verified-pages/src/pages/subscription-confirmed/en.ts (1)
en(1-11)
libs/verified-pages/src/pages/pending-subscriptions/index.test.ts (1)
libs/verified-pages/src/pages/pending-subscriptions/index.ts (2)
GET(147-147)POST(148-148)
libs/subscriptions/src/validation/validation.ts (1)
libs/subscriptions/src/repository/queries.ts (1)
findSubscriptionByUserAndLocation(14-23)
apps/postgres/src/schema-discovery.test.ts (1)
apps/postgres/src/schema-discovery.ts (1)
getPrismaSchemas(5-7)
libs/verified-pages/src/pages/subscription-confirmed/cy.ts (2)
libs/subscriptions/src/locales/cy.ts (1)
cy(1-7)libs/verified-pages/src/pages/unsubscribe-confirmation/cy.ts (1)
cy(1-11)
libs/subscriptions/src/repository/service.ts (2)
libs/subscriptions/src/validation/validation.ts (1)
validateLocationId(4-7)libs/subscriptions/src/repository/queries.ts (5)
findSubscriptionByUserAndLocation(14-23)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)findSubscriptionsByUserId(3-12)deleteSubscriptionRecord(48-52)
libs/verified-pages/src/pages/subscription-confirmed/en.ts (6)
libs/subscriptions/src/locales/en.ts (1)
en(1-7)libs/verified-pages/src/pages/delete-subscription/en.ts (1)
en(1-9)libs/verified-pages/src/pages/location-name-search/en.ts (1)
en(1-24)libs/verified-pages/src/pages/pending-subscriptions/en.ts (1)
en(1-12)libs/verified-pages/src/pages/subscription-management/en.ts (1)
en(1-10)libs/verified-pages/src/pages/unsubscribe-confirmation/en.ts (1)
en(1-11)
libs/verified-pages/src/pages/unsubscribe-confirmation/index.ts (8)
libs/verified-pages/src/pages/unsubscribe-confirmation/cy.ts (1)
cy(1-11)libs/verified-pages/src/pages/unsubscribe-confirmation/en.ts (1)
en(1-11)libs/subscriptions/src/repository/service.ts (1)
removeSubscription(36-50)libs/verified-pages/src/pages/delete-subscription/index.ts (1)
GET(134-134)libs/verified-pages/src/pages/location-name-search/index.ts (1)
GET(220-220)libs/verified-pages/src/pages/pending-subscriptions/index.ts (1)
GET(147-147)libs/verified-pages/src/pages/subscription-confirmed/index.ts (1)
GET(44-44)libs/verified-pages/src/pages/subscription-management/index.ts (1)
GET(67-67)
libs/verified-pages/src/pages/subscription-management/cy.ts (6)
libs/subscriptions/src/locales/cy.ts (1)
cy(1-7)libs/verified-pages/src/pages/delete-subscription/cy.ts (1)
cy(1-9)libs/verified-pages/src/pages/location-name-search/cy.ts (1)
cy(1-24)libs/verified-pages/src/pages/pending-subscriptions/cy.ts (1)
cy(1-12)libs/verified-pages/src/pages/subscription-confirmed/cy.ts (1)
cy(1-11)libs/verified-pages/src/pages/unsubscribe-confirmation/cy.ts (1)
cy(1-11)
libs/verified-pages/src/pages/delete-subscription/index.ts (4)
libs/verified-pages/src/pages/delete-subscription/cy.ts (1)
cy(1-9)libs/verified-pages/src/pages/delete-subscription/en.ts (1)
en(1-9)libs/subscriptions/src/repository/queries.ts (1)
findSubscriptionById(42-46)libs/verified-pages/src/pages/subscription-management/index.ts (1)
GET(67-67)
libs/verified-pages/src/pages/pending-subscriptions/index.ts (4)
libs/verified-pages/src/pages/pending-subscriptions/cy.ts (1)
cy(1-12)libs/verified-pages/src/pages/pending-subscriptions/en.ts (1)
en(1-12)libs/subscriptions/src/repository/service.ts (1)
replaceUserSubscriptions(65-100)libs/verified-pages/src/pages/subscription-confirmed/index.ts (1)
GET(44-44)
libs/verified-pages/src/pages/pending-subscriptions/en.ts (6)
libs/subscriptions/src/locales/en.ts (1)
en(1-7)libs/verified-pages/src/pages/delete-subscription/en.ts (1)
en(1-9)libs/verified-pages/src/pages/location-name-search/en.ts (1)
en(1-24)libs/verified-pages/src/pages/subscription-confirmed/en.ts (1)
en(1-11)libs/verified-pages/src/pages/subscription-management/en.ts (1)
en(1-10)libs/verified-pages/src/pages/unsubscribe-confirmation/en.ts (1)
en(1-11)
libs/subscriptions/src/repository/queries.test.ts (1)
libs/subscriptions/src/repository/queries.ts (6)
findSubscriptionsByUserId(3-12)findSubscriptionByUserAndLocation(14-23)findSubscriptionById(42-46)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)deleteSubscriptionRecord(48-52)
libs/subscriptions/src/validation/validation.test.ts (1)
libs/subscriptions/src/validation/validation.ts (2)
validateLocationId(4-7)validateDuplicateSubscription(9-12)
libs/subscriptions/src/repository/service.test.ts (1)
libs/subscriptions/src/repository/service.ts (5)
createSubscription(13-30)getSubscriptionsByUserId(32-34)removeSubscription(36-50)createMultipleSubscriptions(52-63)replaceUserSubscriptions(65-100)
libs/verified-pages/src/pages/unsubscribe-confirmation/en.ts (1)
libs/verified-pages/src/pages/subscription-confirmed/en.ts (1)
en(1-11)
libs/verified-pages/src/pages/location-name-search/index.test.ts (6)
libs/verified-pages/src/pages/delete-subscription/index.ts (2)
GET(134-134)POST(135-135)libs/verified-pages/src/pages/location-name-search/index.ts (2)
GET(220-220)POST(221-221)libs/verified-pages/src/pages/pending-subscriptions/index.ts (2)
GET(147-147)POST(148-148)libs/verified-pages/src/pages/subscription-confirmed/index.ts (1)
GET(44-44)libs/verified-pages/src/pages/subscription-management/index.ts (1)
GET(67-67)libs/verified-pages/src/pages/unsubscribe-confirmation/index.ts (1)
GET(43-43)
libs/verified-pages/src/pages/subscription-management/index.test.ts (6)
libs/verified-pages/src/pages/delete-subscription/index.ts (1)
GET(134-134)libs/verified-pages/src/pages/location-name-search/index.ts (1)
GET(220-220)libs/verified-pages/src/pages/pending-subscriptions/index.ts (1)
GET(147-147)libs/verified-pages/src/pages/subscription-confirmed/index.ts (1)
GET(44-44)libs/verified-pages/src/pages/subscription-management/index.ts (1)
GET(67-67)libs/verified-pages/src/pages/unsubscribe-confirmation/index.ts (1)
GET(43-43)
libs/subscriptions/src/locales/cy.ts (2)
libs/verified-pages/src/pages/location-name-search/cy.ts (1)
cy(1-24)libs/verified-pages/src/pages/pending-subscriptions/cy.ts (1)
cy(1-12)
libs/verified-pages/src/pages/delete-subscription/index.test.ts (1)
libs/verified-pages/src/pages/delete-subscription/index.ts (2)
GET(134-134)POST(135-135)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
libs/verified-pages/src/pages/pending-subscriptions/index.ts (3)
73-74: Unsafe session access remains unaddressed.Line 74 directly writes to
req.session.emailSubscriptions.pendingSubscriptionswithout ensuringemailSubscriptionsis initialized. If the session object is missing this property, this will throw a TypeError.As previously noted, add a guard before the assignment:
if (action === "remove" && locationId) { + if (!req.session.emailSubscriptions) { + req.session.emailSubscriptions = { pendingSubscriptions: [] }; + } req.session.emailSubscriptions.pendingSubscriptions = pendingLocationIds.filter((id: string) => id !== locationId);
108-110: Same unsafe session access pattern in confirm action.Lines 108-110 directly assign to
req.session.emailSubscriptionsproperties without checking ifemailSubscriptionsexists. Apply the same guard as recommended for the remove action.
96-148: POST handler still lacks a default branch for unknown actions.If
actionis neither"remove"nor"confirm", the handler completes without sending a response, leaving the request hanging.Add a fallback at the end of the handler:
}); } } + + return res.redirect("/pending-subscriptions"); };
🧹 Nitpick comments (7)
libs/verified-pages/src/pages/subscription-management/index.ts (3)
12-14: Redundant auth check afterrequireAuth()middleware.The
requireAuth()middleware in the export chain (line 67) should already handle unauthenticated requests. This check is defensive but adds redundancy.If
requireAuth()guaranteesreq.user.idexists, you can simplify:- if (!req.user?.id) { - return res.redirect("/sign-in"); - } - - const userId = req.user.id; + const userId = req.user!.id;Alternatively, if keeping the check for safety, ensure the function return type is consistent with
Promise<void>by removing the return value:if (!req.user?.id) { - return res.redirect("/sign-in"); + res.redirect("/sign-in"); + return; }
39-42: Navigation setup is duplicated.Consider extracting navigation initialization to reduce code duplication between the try and catch blocks.
You can set navigation before the try block or extract a helper:
+ const setNavigation = () => { + if (!res.locals.navigation) { + res.locals.navigation = {}; + } + res.locals.navigation.verifiedItems = buildVerifiedUserNavigation(req.path, locale); + }; + try { const subscriptions = await getSubscriptionsByUserId(userId); // ... - if (!res.locals.navigation) { - res.locals.navigation = {}; - } - res.locals.navigation.verifiedItems = buildVerifiedUserNavigation(req.path, locale); + setNavigation(); res.render("subscription-management/index", { // ... }); } catch (error) { console.error("Error retrieving subscriptions:", error); - if (!res.locals.navigation) { - res.locals.navigation = {}; - } - res.locals.navigation.verifiedItems = buildVerifiedUserNavigation(req.path, locale); + setNavigation(); res.render("subscription-management/index", { // ... }); }Also applies to: 53-56
48-48: Avoidanytype assertion.Using
(req as any)violates strict mode guidelines. Consider properly typing the CSRF token access.Define an interface for the extended request or use a type guard:
interface CsrfRequest extends Request { csrfToken?: () => string; }Then cast appropriately:
- csrfToken: (req as any).csrfToken?.() || "" + csrfToken: (req as CsrfRequest).csrfToken?.() || ""Apply the same fix at line 62.
libs/verified-pages/src/pages/pending-subscriptions/index.test.ts (2)
163-179: Consider consolidating or differentiating this test from the earlier confirm test.This test ("should preserve existing subscriptions when adding new ones") has identical assertions to the test at lines 119-135. Both call
replaceUserSubscriptionswith["123", "456", "789"]and redirect to/subscription-confirmed. Consider either:
- Removing this duplicate test, or
- Differentiating it by testing a scenario where existing subscriptions overlap with pending ones to verify deduplication works
95-180: Add test for unknown action value.The POST tests don't cover the case where
actionis neither"remove"nor"confirm"(e.g., missing, empty, or an unexpected value). Adding such a test would help verify the handler responds appropriately in edge cases.it("should handle unknown action gracefully", async () => { mockReq.body = { action: "unknown" }; await POST[POST.length - 1](mockReq as Request, mockRes as Response, vi.fn()); expect(mockRes.redirect).toHaveBeenCalledWith("/pending-subscriptions"); });libs/verified-pages/src/pages/pending-subscriptions/index.ts (1)
31-43: Consider extracting duplicate location-fetching logic into a helper.The
pendingLocationsresolution logic (Promise.all with getLocationById and filtering) is duplicated in both the GET handler and the POST error handler. Extracting this into a helper function would reduce duplication.const resolvePendingLocations = async (pendingIds: string[], locale: string) => { return ( await Promise.all( pendingIds.map(async (id: string) => { const location = await getLocationById(Number.parseInt(id, 10)); return location ? { locationId: id, name: locale === "cy" ? location.welshName : location.name } : null; }) ) ).filter(Boolean); };Also applies to: 116-128
libs/subscriptions/prisma/schema.prisma (1)
11-24: Add explicit referential actions to location relation.The
locationrelation (line 18) should explicitly specifyonDeleteandonUpdatebehavior to match the migration's foreign key constraint (apps/postgres/prisma/migrations/20251127162616_change_subscription_location_id_to_int/migration.sql:20) which usesON DELETE RESTRICT ON UPDATE CASCADE.While Prisma will honor the database-level constraint, explicitly specifying these actions in the schema ensures consistency, prevents drift, and makes the intended behavior clear to developers.
Apply this diff:
- location Location @relation(fields: [locationId], references: [locationId]) + location Location @relation(fields: [locationId], references: [locationId], onDelete: Restrict, onUpdate: Cascade)
📜 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 (13)
apps/postgres/prisma/migrations/20251127162616_change_subscription_location_id_to_int/migration.sql(1 hunks)libs/location/prisma/schema.prisma(1 hunks)libs/subscriptions/prisma/schema.prisma(1 hunks)libs/subscriptions/src/repository/queries.test.ts(1 hunks)libs/subscriptions/src/repository/queries.ts(1 hunks)libs/subscriptions/src/repository/service.test.ts(1 hunks)libs/subscriptions/src/repository/service.ts(1 hunks)libs/subscriptions/src/validation/validation.test.ts(1 hunks)libs/subscriptions/src/validation/validation.ts(1 hunks)libs/verified-pages/src/pages/pending-subscriptions/index.test.ts(1 hunks)libs/verified-pages/src/pages/pending-subscriptions/index.ts(1 hunks)libs/verified-pages/src/pages/subscription-management/index.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/subscriptions/src/validation/validation.ts
- libs/subscriptions/src/repository/service.test.ts
- libs/subscriptions/src/repository/service.ts
🧰 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 should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and Interfaces must use PascalCase (e.g.,UserService,CaseRepository). Do NOT useIprefix for interfaces (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. Don't use a class 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/subscriptions/src/repository/queries.test.tslibs/subscriptions/src/validation/validation.test.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/subscriptions/src/repository/queries.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/subscription-management/index.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/subscriptions/src/repository/queries.test.tslibs/subscriptions/src/validation/validation.test.tslibs/verified-pages/src/pages/pending-subscriptions/index.tslibs/subscriptions/src/repository/queries.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/subscription-management/index.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All packages must use
"test": "vitest run"as the test script. Unit and integration tests must be co-located with source code as*.test.tsfiles.
Files:
libs/subscriptions/src/repository/queries.test.tslibs/subscriptions/src/validation/validation.test.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.ts
**/*.prisma
📄 CodeRabbit inference engine (CLAUDE.md)
Database tables and fields MUST be singular and snake_case (e.g.,
user,case,created_at). Use Prisma@@mapand@mapfor aliases.
Files:
libs/location/prisma/schema.prismalibs/subscriptions/prisma/schema.prisma
**/{pages,locales}/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
Every page must support both English and Welsh by providing
enandcyobjects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/subscription-management/index.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Page routes are created based on file names within the
pages/directory. Nested routes are created using subdirectories (e.g.,pages/admin/my-page.tsbecomes/admin/my-page).
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/subscription-management/index.ts
**/{locales,pages}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/pending-subscriptions/index.test.tslibs/verified-pages/src/pages/subscription-management/index.ts
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
All package.json files must use"type": "module"to enforce ES modules. Never use CommonJSrequire()ormodule.exports. Useimport/exportonly.
Express version 5.x only must be used ("express": "5.1.0"). Pin all dependencies to specific versions except peer dependencies.
Build scripts must includebuild:nunjucksif the module contains Nunjucks templates in thepages/directory to copy .njk files to dist.
Files:
package.json
🧠 Learnings (4)
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.
Applied to files:
libs/subscriptions/src/validation/validation.test.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.
Applied to files:
libs/verified-pages/src/pages/pending-subscriptions/index.tslibs/verified-pages/src/pages/subscription-management/index.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/{locales,pages}/**/*.ts : Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.
Applied to files:
libs/verified-pages/src/pages/pending-subscriptions/index.ts
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/package.json : Express version 5.x only must be used (`"express": "5.1.0"`). Pin all dependencies to specific versions except peer dependencies.
Applied to files:
package.json
🧬 Code graph analysis (4)
libs/subscriptions/src/repository/queries.test.ts (1)
libs/subscriptions/src/repository/queries.ts (6)
findSubscriptionsByUserId(3-12)findSubscriptionByUserAndLocation(14-23)findSubscriptionById(42-46)countSubscriptionsByUserId(25-31)createSubscriptionRecord(33-40)deleteSubscriptionRecord(48-52)
libs/subscriptions/src/validation/validation.test.ts (1)
libs/subscriptions/src/validation/validation.ts (2)
validateLocationId(4-7)validateDuplicateSubscription(9-13)
libs/verified-pages/src/pages/pending-subscriptions/index.test.ts (1)
libs/verified-pages/src/pages/pending-subscriptions/index.ts (2)
GET(151-151)POST(152-152)
libs/verified-pages/src/pages/subscription-management/index.ts (3)
libs/verified-pages/src/pages/subscription-management/cy.ts (1)
cy(1-10)libs/verified-pages/src/pages/subscription-management/en.ts (1)
en(1-10)libs/subscriptions/src/repository/service.ts (1)
getSubscriptionsByUserId(33-35)
⏰ 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 (16)
libs/verified-pages/src/pages/subscription-management/index.ts (2)
1-6: LGTM on imports.Imports correctly use workspace aliases (
@hmcts/*) and include.jsextensions for relative imports as required for ESM with Node.js "nodenext" module resolution.
67-67: Export structure looks correct.The middleware chain with
requireAuth(),blockUserAccess(), and the handler follows the established pattern for verified pages. Based on learnings, page controllers should export GET/POST with proper signature—this array-based approach correctly applies middleware before the handler.libs/verified-pages/src/pages/pending-subscriptions/index.test.ts (4)
1-4: LGTM on imports.Imports are correctly structured with workspace aliases and
.jsextensions for ESM compatibility as per coding guidelines.
6-23: LGTM on mock setup.Mocks appropriately isolate the handlers from external dependencies while providing realistic return shapes.
29-45: LGTM on test setup.The mock request and response objects are well-structured with the required properties for testing both handlers.
51-93: LGTM on GET handler tests.Tests cover the key scenarios: multiple subscriptions with plurality, empty state with error rendering, and single subscription handling.
libs/verified-pages/src/pages/pending-subscriptions/index.ts (4)
1-6: LGTM on imports.Imports follow coding guidelines with workspace aliases and
.jsextensions for ESM compatibility.
8-58: LGTM on GET handler logic.The handler correctly handles both empty and populated pending subscriptions, with proper locale support and parallel location fetching.
64-68: LGTM on authentication check.Proper authentication guard that redirects to sign-in when user is not authenticated, eliminating the previous test-user-id fallback concern.
151-152: LGTM on exports with auth middleware.The handlers are correctly wrapped with
requireAuth()andblockUserAccess()middleware, addressing the previous concern about missing authentication protection.apps/postgres/prisma/migrations/20251127162616_change_subscription_location_id_to_int/migration.sql (1)
7-20: Migration approach looks sound.The migration correctly:
- Drops indexes before altering the column to avoid constraint violations
- Uses explicit cast with
USING "location_id"::INTEGERto convert existing data- Recreates indexes after the type change
- Adds proper foreign key constraint with
ON DELETE RESTRICT ON UPDATE CASCADENote: This migration will fail if existing data contains non-numeric TEXT values. Ensure data validation is complete before running this migration.
libs/subscriptions/src/validation/validation.test.ts (1)
1-69: LGTM!The test coverage is comprehensive and follows best practices:
- Properly mocks external dependencies (@hmcts/location and repository queries)
- Uses
.jsextension for relative imports per coding guidelines- Covers success cases and edge cases (invalid IDs, non-numeric input, missing records)
- Validates both function behavior and correct usage of mocked dependencies
libs/location/prisma/schema.prisma (1)
52-52: LGTM!The
subscriptionsrelation properly establishes the one-to-many relationship between Location and Subscription. The corresponding relation is correctly defined in the Subscription model (libs/subscriptions/prisma/schema.prisma:18).libs/subscriptions/src/repository/queries.test.ts (1)
1-218: LGTM!Excellent test coverage with proper structure:
- Comprehensive coverage of all query functions (find, count, create, delete)
- Properly mocks Prisma client operations
- Tests both success paths and edge cases (empty results, null returns)
- Validates correct query parameters and return values
- Uses
.jsextension for relative imports per coding guidelines- Implements proper test cleanup with beforeEach/afterEach
libs/subscriptions/src/repository/queries.ts (1)
1-52: LGTM!Clean repository layer implementation following best practices:
- Uses parameterized Prisma queries to prevent SQL injection ✓
- No sensitive data logging ✓
- Pure functions with no side effects ✓
- Proper use of Prisma unique constraints (
unique_user_locationat line 17)- Self-documenting code without unnecessary comments ✓
- Workspace alias import per coding guidelines ✓
package.json (1)
66-66: Node-forge 1.3.2 is the current latest version on NPM with no known security advisories.Verification confirms that version 1.3.2 is appropriately pinned:
- It is the latest available version (published 2025-11-25)
- Marked as the current stable release on NPM
- No security advisories are documented for this version
- The resolution correctly pins to a specific version per guidelines
The change is valid and requires no action.
...stgres/prisma/migrations/20251127162616_change_subscription_location_id_to_int/migration.sql
Show resolved
Hide resolved
|
2 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-192
https://tools.hmcts.net/jira/browse/VIBE-196
Change description
Add Subscription for Verified Users and also remove subscription functionality
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.