VIBE-362: Add user management feature for system administrators #310#335
VIBE-362: Add user management feature for system administrators #310#335alexbottenberg wants to merge 48 commits intomasterfrom
Conversation
Implements comprehensive user search, management, and deletion functionality for system admin users. Includes advanced filtering with removable filter tags using GOV.UK Design System components. Features: - User search with filters (email, user ID, provenance ID, roles, provenances) - Removable filter tags with MOJ pattern styling - Server-side pagination (25 users per page) - User management page showing full user details - User deletion with confirmation workflow - Self-deletion prevention - Bilingual support (English/Welsh) - Database indexes for improved search performance Technical details: - Dynamic routes using [userId] parameter pattern - Session-based filter persistence - GOV.UK Tag component for filter display - Enhanced error logging with structured context - All 342 tests passing Resolves #310 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a bilingual System Admin User Management feature (find/manage/delete users) with server-rendered handlers, Prisma-backed search/pagination and transactional deletion, validators, session-backed filters, templates/styles, unit and E2E tests, DB migrations/indexes, and supporting subscription-list-type functionality and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as System Admin
participant Browser as Browser
participant Server as Express App
participant DB as Prisma / Database
Admin->>Browser: Request Find users (GET /find-users?lng=...)
Browser->>Server: GET /find-users?lng=...
Server->>DB: searchUsers(filters, page)
DB-->>Server: paginated results
Server-->>Browser: Render find-users page
Admin->>Browser: Open Manage user (GET /manage-user/[userId]?lng=...)
Browser->>Server: GET /manage-user/[userId]?lng=...
Server->>DB: getUserById(userId)
DB-->>Server: user details
Server-->>Browser: Render manage-user page
Admin->>Browser: Open Delete confirm (GET /delete-user-confirm/[userId]?lng=...)
Browser->>Server: GET /delete-user-confirm/[userId]?lng=...
Server->>DB: getUserById(userId)
DB-->>Server: user details
Server-->>Browser: Render delete confirmation
Admin->>Browser: Submit "Yes" (POST /delete-user-confirm/[userId])
Browser->>Server: POST /delete-user-confirm/[userId]
Server->>DB: $transaction -> delete related records -> delete user
DB-->>Server: transaction committed
Server-->>Browser: Redirect /delete-user-success?lng=...
Browser->>Server: GET /delete-user-success?lng=...
Server-->>Browser: Render success page
Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/system-admin-pages/src/pages/system-admin-dashboard/cy.ts (1)
27-27:⚠️ Potential issue | 🟡 MinorInconsistent href between English and Welsh locales.
The Blob Explorer tile has
href: "/blob-explorer"here, but the English locale hashref: "/blob-explorer-locations". This may cause navigation issues when switching languages.Suggested fix
{ title: "Archwiliwr Blob", description: "Darganfod cynnwys wedi'i uwchlwytho i bob lleoliad", - href: "/blob-explorer" + href: "/blob-explorer-locations" },
🧹 Nitpick comments (14)
libs/system-admin-pages/src/pages/find-users-clear-filters/index.ts (2)
17-17: Unnecessaryasyncmodifier.The handler contains no
awaitstatements, soasyncis superfluous.Suggested fix
-const getHandler = async (req: Request, res: Response) => { +const getHandler = (req: Request, res: Response) => {
4-15: Extract theUserManagementSessioninterface to a shared types file.The interface is duplicated identically across
find-users,find-users-remove-filter, andfind-users-clear-filters. Moving it to a shared location within the system-admin-pages package would reduce duplication and improve maintainability.libs/system-admin-pages/src/pages/find-users/cy.ts (1)
25-25: Consider Welsh pluralisation forresultsCount.The English version handles singular/plural (
user/users), but the Welsh version uses a single form. Welsh has different pluralisation rules—verify with a Welsh speaker whether "defnyddiwr" (singular) or "defnyddwyr" (plural) should be used conditionally.libs/system-admin-pages/src/user-management/validation.ts (2)
1-1: Remove unusedEMAIL_REGEXconstant.This regex is defined but never used. The comment on Line 24 confirms email format validation was intentionally omitted to allow partial search matching.
🧹 Proposed fix
-const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; const ALPHANUMERIC_REGEX = /^[a-zA-Z0-9]+$/;
133-148: Minor duplication in error object.Both branches return identical error objects. Consider extracting or simplifying.
♻️ Proposed simplification
export function validateDeleteConfirmation(confirmation: string | undefined): ValidationError | null { - if (!confirmation) { - return { - text: "Select yes or no to continue", - href: "#confirmation" - }; - } - - if (confirmation !== "yes" && confirmation !== "no") { + if (confirmation !== "yes" && confirmation !== "no") { return { text: "Select yes or no to continue", href: "#confirmation" }; } return null; }libs/system-admin-pages/src/pages/find-users-remove-filter/index.ts (1)
4-15: ConsolidateUserManagementSessioninterface to a shared types file.This interface is duplicated identically across three page handlers:
find-users/index.ts,find-users-remove-filter/index.ts, andfind-users-clear-filters/index.ts. Create a shared types file (e.g.,libs/system-admin-pages/src/pages/types.ts) and import it from all three modules to reduce duplication and ease future maintenance.apps/postgres/prisma/schema.prisma (1)
43-45: Email index strategy does not optimise the current search pattern. Email filtering usescontainswith case-insensitive mode, which translates to SQL ILIKE with leading wildcards (e.g.'%test%'). A standard B-tree index onlibs/system-admin-pages/src/user-management/queries.ts (1)
19-29: Consider aligning with existing User interface.The
Userinterface here usesstringforroleanduserProvenance, whilstlibs/account/src/repository/model.tsdefines these as union types ("VERIFIED" | "LOCAL_ADMIN" | ...). Using loose string types may bypass compile-time checks.libs/system-admin-pages/src/pages/manage-user/[userId]/index.njk (1)
11-14: Consider localising the iconFallbackText.The
iconFallbackText: "Warning"is hardcoded in English. For bilingual support, this should come from the content object (e.g.,iconFallbackText).♻️ Proposed fix
{{ govukWarningText({ text: warningText, - iconFallbackText: "Warning" + iconFallbackText: warningIconFallbackText }) }}libs/system-admin-pages/src/assets/css/user-management.scss (2)
1-93: Consider using GOV.UK colour variables instead of hardcoded hex values.Multiple hardcoded colours (
#b1b4b6,#f3f2f1,#003078,#fd0,#0b0c0c) could use GOV.UK Design System variables (e.g.,govuk-colour("mid-grey"),govuk-colour("light-grey")) for consistency and maintainability.
1-54: Excessive use of!importantoverrides.The heavy reliance on
!importantsuggests conflicts with GOV.UK default styling. Consider adjusting selector specificity or component structure rather than forcing overrides, which can complicate future maintenance.libs/system-admin-pages/src/pages/find-users/index.test.ts (1)
11-13: Avoidanyfor the session mock. Line 13 drops type safety; a small interface keeps tests strict and self-documenting.Suggested fix
+type MockSession = { + userManagement?: { + filters?: { + email?: string; + userId?: string; + userProvenanceId?: string; + roles?: string[]; + provenances?: string[]; + }; + page?: number; + }; +}; + - let mockSession: any; + let mockSession: MockSession;As per coding guidelines: Use TypeScript strict mode. Avoid
anywithout justification.libs/system-admin-pages/src/user-management/queries.test.ts (1)
25-130: Consider adding tests foruserProvenanceIdandprovenancesfilters.The
searchUserstests cover email, userId, roles, and pagination, but omituserProvenanceIdexact-match andprovenancesarray filters. Adding these would complete filter coverage.libs/system-admin-pages/src/pages/find-users/index.ts (1)
8-19: Move interface to bottom of module.Per coding guidelines, interfaces and types should be placed at the bottom of the module.
♻️ Suggested reordering
Move the
UserManagementSessioninterface to the end of the file, after the exports on lines 221-222.As per coding guidelines: "Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom."
| **Deletion Strategy**: Soft delete approach initially, with cascade deletion of related subscriptions | ||
|
|
||
| **Navigation State**: Preserve search filters and pagination when returning from Manage page via session storage |
There was a problem hiding this comment.
Align deletion strategy wording with the requirement. Line 29 states “Soft delete approach initially”, but the objectives specify deleted users must be removed from the database. Please update this section to match the agreed behaviour.
| ### 4. Missing Database Indexes | ||
| **Location**: Database schema | ||
| **Problem**: No indexes exist on frequently queried fields (email, role, userProvenance). | ||
| **Impact**: Poor query performance with large user datasets, potential timeout on searches. | ||
| **Solution**: Add indexes in Prisma schema: | ||
| ```prisma | ||
| model User { | ||
| // ... | ||
| @@index([email]) | ||
| @@index([role]) | ||
| @@index([userProvenance]) | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Update the review narrative if indexes are already added. Lines 71-83 list missing indexes as critical, but the PR objectives mention indexes have been introduced. Please reconcile this section to avoid false negatives in the review note.
| ## Database Layer | ||
|
|
||
| - [ ] Add database indexes for user search (email, role, userProvenance) | ||
| - [x] Create `src/user-management/queries.ts` with searchUsers function | ||
| - [x] Add deleteUserById function to `src/user-management/queries.ts` | ||
| - [x] Add getUserById function to `src/user-management/queries.ts` | ||
| - [x] Add unit tests for all query functions in `src/user-management/queries.test.ts` |
There was a problem hiding this comment.
Mark the index task as complete.
The migration adds these indexes, so the checklist should be ticked.
Suggested fix
- - [ ] Add database indexes for user search (email, role, userProvenance)
+ - [x] Add database indexes for user search (email, role, userProvenance)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Database Layer | |
| - [ ] Add database indexes for user search (email, role, userProvenance) | |
| - [x] Create `src/user-management/queries.ts` with searchUsers function | |
| - [x] Add deleteUserById function to `src/user-management/queries.ts` | |
| - [x] Add getUserById function to `src/user-management/queries.ts` | |
| - [x] Add unit tests for all query functions in `src/user-management/queries.test.ts` | |
| ## Database Layer | |
| - [x] Add database indexes for user search (email, role, userProvenance) | |
| - [x] Create `src/user-management/queries.ts` with searchUsers function | |
| - [x] Add deleteUserById function to `src/user-management/queries.ts` | |
| - [x] Add getUserById function to `src/user-management/queries.ts` | |
| - [x] Add unit tests for all query functions in `src/user-management/queries.test.ts` |
| * on the dashboard, when the system admin user clicks on the 'User Management' tab, then the system admin user is taken to the 'Find, update and delete a user' page and can view the list of CaTH users in a table that displays the following columns in sequential order 'Email', 'Role' and 'Provenance'. The 4th column without a title provides a link to 'Manage' each user provided in the table rows. Where there are several users, then a maximum of 25 users are displayed per page | ||
| * A filter panel is provided on the left side of the page. The first section displays the 'Selected filters'. 2nd section displays 3 free text search fields. First is the 'Email' search field. When the correct search criteria is inputted and run, then the user list is updated to display matching results; however, where the wrong email is inputted, the following error message is displayed; '{**}There is a problem'. '{**}No users could be found matching your search criteria. Try adjusting or clearing the filters.' The second and third search fields are the 'User ID' and 'User Provenance ID' fields which display the following descriptive message beneath the field name and above the search bar 'Must be an exact match'. The 3rd section provides 2 check box sub-sections titled 'Role' which has the following options, 'Verified, CTSC Admin, Local[+] Admin and System Admin', and then 'Provenance' with the following options 'CFT IdAM and SSO'. [/+][-]Admin, CTSC Super Admin, Local Super Admin and System Admin', and then 'Provenance' with the following options 'B2C, CFT IdAM, Crime IdAM and SSO'.[/-] | ||
| * Clicking on the 'Manage' link beside a user takes the system admin to the 'Manage <user email>' page which displays a caution symbol underneath the page title with the following descriptive message beside it; '{**}Ensure authorisation has been granted before updating this user'{**}. The following rows are provided in a displayed table; 'User ID, Email, Role. Provenance, Provenance ID, Creation Date and Last sign in'. This is followed by a red 'Delete user' button. |
There was a problem hiding this comment.
Remove stray diff artefacts in the acceptance criteria.
The bracketed [+]/[-] markers look like leftover diff text and break the sentence.
Suggested fix
-* A filter panel is provided on the left side of the page. The first section displays the 'Selected filters'. 2nd section displays 3 free text search fields. First is the 'Email' search field. When the correct search criteria is inputted and run, then the user list is updated to display matching results; however, where the wrong email is inputted, the following error message is displayed; '{**}There is a problem'. '{**}No users could be found matching your search criteria. Try adjusting or clearing the filters.' The second and third search fields are the 'User ID' and 'User Provenance ID' fields which display the following descriptive message beneath the field name and above the search bar 'Must be an exact match'. The 3rd section provides 2 check box sub-sections titled 'Role' which has the following options, 'Verified, CTSC Admin, Local[+] Admin and System Admin', and then 'Provenance' with the following options 'CFT IdAM and SSO'. [/+][-]Admin, CTSC Super Admin, Local Super Admin and System Admin', and then 'Provenance' with the following options 'B2C, CFT IdAM, Crime IdAM and SSO'.[/-]
+* A filter panel is provided on the left side of the page. The first section displays the 'Selected filters'. 2nd section displays 3 free text search fields. First is the 'Email' search field. When the correct search criteria is inputted and run, then the user list is updated to display matching results; however, where the wrong email is inputted, the following error message is displayed; '{**}There is a problem'. '{**}No users could be found matching your search criteria. Try adjusting or clearing the filters.' The second and third search fields are the 'User ID' and 'User Provenance ID' fields which display the following descriptive message beneath the field name and above the search bar 'Must be an exact match'. The 3rd section provides 2 check box sub-sections titled 'Role' which has the following options, 'Verified, CTSC Admin, Local Admin and System Admin', and then 'Provenance' with the following options 'B2C, CFT IdAM, Crime IdAM and SSO'.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * on the dashboard, when the system admin user clicks on the 'User Management' tab, then the system admin user is taken to the 'Find, update and delete a user' page and can view the list of CaTH users in a table that displays the following columns in sequential order 'Email', 'Role' and 'Provenance'. The 4th column without a title provides a link to 'Manage' each user provided in the table rows. Where there are several users, then a maximum of 25 users are displayed per page | |
| * A filter panel is provided on the left side of the page. The first section displays the 'Selected filters'. 2nd section displays 3 free text search fields. First is the 'Email' search field. When the correct search criteria is inputted and run, then the user list is updated to display matching results; however, where the wrong email is inputted, the following error message is displayed; '{**}There is a problem'. '{**}No users could be found matching your search criteria. Try adjusting or clearing the filters.' The second and third search fields are the 'User ID' and 'User Provenance ID' fields which display the following descriptive message beneath the field name and above the search bar 'Must be an exact match'. The 3rd section provides 2 check box sub-sections titled 'Role' which has the following options, 'Verified, CTSC Admin, Local[+] Admin and System Admin', and then 'Provenance' with the following options 'CFT IdAM and SSO'. [/+][-]Admin, CTSC Super Admin, Local Super Admin and System Admin', and then 'Provenance' with the following options 'B2C, CFT IdAM, Crime IdAM and SSO'.[/-] | |
| * Clicking on the 'Manage' link beside a user takes the system admin to the 'Manage <user email>' page which displays a caution symbol underneath the page title with the following descriptive message beside it; '{**}Ensure authorisation has been granted before updating this user'{**}. The following rows are provided in a displayed table; 'User ID, Email, Role. Provenance, Provenance ID, Creation Date and Last sign in'. This is followed by a red 'Delete user' button. | |
| * on the dashboard, when the system admin user clicks on the 'User Management' tab, then the system admin user is taken to the 'Find, update and delete a user' page and can view the list of CaTH users in a table that displays the following columns in sequential order 'Email', 'Role' and 'Provenance'. The 4th column without a title provides a link to 'Manage' each user provided in the table rows. Where there are several users, then a maximum of 25 users are displayed per page | |
| * A filter panel is provided on the left side of the page. The first section displays the 'Selected filters'. 2nd section displays 3 free text search fields. First is the 'Email' search field. When the correct search criteria is inputted and run, then the user list is updated to display matching results; however, where the wrong email is inputted, the following error message is displayed; '{**}There is a problem'. '{**}No users could be found matching your search criteria. Try adjusting or clearing the filters.' The second and third search fields are the 'User ID' and 'User Provenance ID' fields which display the following descriptive message beneath the field name and above the search bar 'Must be an exact match'. The 3rd section provides 2 check box sub-sections titled 'Role' which has the following options, 'Verified, CTSC Admin, Local Admin and System Admin', and then 'Provenance' with the following options 'B2C, CFT IdAM, Crime IdAM and SSO'. | |
| * Clicking on the 'Manage' link beside a user takes the system admin to the 'Manage <user email>' page which displays a caution symbol underneath the page title with the following descriptive message beside it; '{**}Ensure authorisation has been granted before updating this user'{**}. The following rows are provided in a displayed table; 'User ID, Email, Role. Provenance, Provenance ID, Creation Date and Last sign in'. This is followed by a red 'Delete user' button. |
🧰 Tools
🪛 LanguageTool
[grammar] ~28-~28: The verb form ‘is’ does not seem to match the subject ‘criteria’.
Context: ...field. When the correct search criteria is inputted and run, then the user list is...
(SUBJECT_VERB_AGREEMENT_PLURAL)
[misspelling] ~28-~28: This word is normally spelled as one.
Context: ...'. The 3rd section provides 2 check box sub-sections titled 'Role' which has the following o...
(EN_COMPOUNDS_SUB_SECTIONS)
[style] ~29-~29: Would you like to use the Oxford spelling “authorization”? The spelling ‘authorisation’ is also correct.
Context: ...criptive message beside it; '{**}Ensure authorisation has been granted before updating this u...
(OXFORD_SPELLING_Z_NOT_S)
🪛 markdownlint-cli2 (0.20.0)
[warning] 28-28: Reference links and images should use a label that is defined
Missing link or image reference definition: "-"
(MD052, reference-links-images)
libs/system-admin-pages/src/pages/delete-user-confirm/[userId]/index.test.ts
Show resolved
Hide resolved
libs/system-admin-pages/src/pages/manage-user/[userId]/index.test.ts
Outdated
Show resolved
Hide resolved
🎭 Playwright E2E Test Results250 tests 250 ✅ 24m 15s ⏱️ Results for commit 5b579c5. ♻️ This comment has been updated with latest results. |
Implements end-to-end testing for issue #310 covering: - User search and filtering (email, roles, provenances, user ID) - Filter tag display, removal, and clear functionality - User management page navigation - User deletion workflow with confirmation - Cancel deletion functionality - Empty results state - Accessibility testing with Axe at each page - Welsh translation verification throughout journey - Keyboard navigation and validation error handling Tests follow E2E guidelines by minimizing test count and including all checks (accessibility, Welsh, validation) within complete user journeys. All 4 tests passing locally with Playwright. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
e2e-tests/tests/user-management.spec.ts (4)
39-40: Place top-level constants before helpers to match module ordering.
MovecreatedUsersabove the helper declarations for consistent structure.As per coding guidelines: Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom.
42-57: Avoid duplicating test-user creation logic.
e2e-tests/utils/notification-helpers.ts:18-34already definescreateTestUser; consider extending it to accept role/provenance and reuse it here to keep seeding consistent.
105-116: Prefer role/label-based locators over CSS selectors.
This makes the tests less brittle and aligns with accessibility semantics; apply the same pattern for the filter tag and confirmation radios.♻️ Example refactor
- const filterTag = page.locator(".user-management-filter-tag").first(); - await filterTag.click(); + await page.getByRole("button", { name: /remove email filter/i }).click(); - await page.locator('input[name="roles"][value="VERIFIED"]').check(); - await page.locator('input[name="provenances"][value="CFT_IDAM"]').check(); + await page.getByRole("checkbox", { name: /verified/i }).check(); + await page.getByRole("checkbox", { name: /cft idam/i }).check(); - await page.locator('input[name="confirmation"][value="yes"]').check(); + await page.getByRole("radio", { name: /^yes$/i }).check();As per coding guidelines: E2E tests should use getByRole() first, getByLabel() for inputs, getByText() for text, and getByTestId() as last resort.
Also applies to: 193-194, 230-231
210-285: Consolidate partial journeys or add inline Welsh/A11y checks.
The additional tests split the journey and omit the Welsh/Axe checks performed in the main test; consider folding these into the primary journey (or adding the inline checks) to match the single-journey guidance.As per coding guidelines: E2E tests in Playwright should minimise test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests.
Updates package resolutions to address 3 known vulnerabilities: - @isaacs/brace-expansion: 5.0.0 → 5.0.1 (Critical, CVSS 9.2) Fixes GHSA-7h2j-956f-4vf2 - lodash: 4.17.21 → 4.17.23 (High, CVSS 6.9) Fixes GHSA-xxjr-mmjv-4gpg - tar: 7.5.4 → 7.5.7 (Medium, CVSS 8.2) Fixes GHSA-34x7-hfp2-rc4v These are transitive dependencies resolved via yarn resolutions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Updates the visual presentation of filter sections: - Move outline from individual sections to wrapper for cleaner appearance - Change header background from #b1b4b6 to lighter #f3f2f1 - Add bottom borders to header and selected filters sections for separation - Remove background color from selected filters section This creates a more cohesive visual hierarchy with a single container outline and subtle section dividers. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Updates tests for manage-user and delete-user-confirm pages after restructuring to use dynamic [userId] parameters: - Fix import paths: Change from ../../ to ../../../ to account for additional directory nesting with [userId] subdirectories - Update render path expectations: Change from "manage-user/index" to "manage-user/[userId]/index" and similar for delete-user-confirm - Update manage-user test to expect formatted date strings instead of Date objects, as the controller now formats dates before rendering All 342 tests now passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Consolidates filter-related routes into the find-users directory structure: - Move /find-users-clear-filters to /find-users/clear-filters - Move /find-users-remove-filter to /find-users/remove-filter This creates a more logical hierarchical structure where all find-users functionality is grouped together, making the codebase easier to navigate. Changes: - Created find-users/clear-filters/index.ts subdirectory - Created find-users/remove-filter/index.ts subdirectory - Updated URLs in find-users/index.ts and index.njk - Removed old top-level directories All 342 tests passing. URLs updated to use forward slashes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added 12 new tests to find-users controller (18 tests total): - Database error handling with graceful degradation - Query string pagination parameter handling - Welsh content rendering (lng=cy) - Pagination with previous/next links - Filter tag generation for all filter types - User rows generation with manage links - Welsh language persistence in pagination URLs - Input whitespace trimming - Single value to array conversion for roles/provenances - Session initialization when userManagement missing - Welsh validation error rendering Also removed old top-level route files that were reorganized into find-users subdirectories in previous commit. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Updated system-admin-dashboard E2E tests to match actual implementation: - User Management tile links to /find-users (not /user-management) - Tile description is "Find, update and delete users" (not "Search...") Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
e2e-tests/tests/system-admin-dashboard.spec.ts (5)
21-21: Avoid testing CSS classes.Testing
.govuk-heading-lchecks visual styling rather than functionality. Consider removing this assertion or replacing with a role-based check.Suggested change
- await expect(heading).toHaveClass(/govuk-heading-l/);Based on learnings: "Do NOT test visual styling (font sizes, background colors, margins, padding) or UI design aspects in E2E tests."
62-65: Grid layout test checks visual styling.Testing the 2-column grid structure verifies UI design rather than user functionality. Consider removing this test.
Based on learnings: "Do NOT test visual styling (font sizes, background colors, margins, padding) or UI design aspects in E2E tests."
68-72: Consider inlining accessibility checks into journey tests.Per coding guidelines, accessibility checks using AxeBuilder should be inline within journey tests rather than in separate dedicated tests. This reduces test count and ensures accessibility is validated as part of the user flow.
Based on learnings: "E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests."
12-66: Consider consolidating tests into fewer journey-based tests.The "Content Display" describe block contains 6 separate tests that could be consolidated. Per guidelines, E2E tests should minimise test count with one test per complete user journey. This would reduce test execution overhead and align with the project's testing strategy.
As per coding guidelines: "E2E tests in Playwright should minimize test count with one test per complete user journey."
41-45: PrefergetByRole()over CSS selectors.The guidelines recommend using
getByRole()first, thengetByLabel()for inputs,getByText()for text content, withgetByTestId()as a last resort. The current CSS selector approach (a.admin-tile:has-text(...)) works but is less resilient to implementation changes.Example using getByRole
- const link = page.locator(`a.admin-tile:has-text("${title}")`); + const link = page.getByRole("link", { name: title });As per coding guidelines: "use getByRole() first, getByLabel() for inputs, getByText() for text, and getByTestId() as last resort."
libs/system-admin-pages/src/pages/delete-user-confirm/[userId]/index.test.ts (1)
14-14: Consider adding a type formockUserinstead ofany.A simple inline type or interface would improve type safety and make the test more self-documenting.
Suggested improvement
- let mockUser: any; + let mockUser: { userId: string; email: string };libs/system-admin-pages/src/pages/find-users/index.test.ts (1)
1-28: Avoidanyfor the mock session.Using
anyhides type drift; a minimal session type keeps the tests aligned with the handler contract.Proposed refactor
import { beforeEach, describe, expect, it, vi } from "vitest"; import * as queries from "../../user-management/queries.js"; import { GET, POST } from "./index.js"; +type MockUserManagementSession = { + userManagement?: { + filters?: { + email?: string; + userId?: string; + userProvenanceId?: string; + roles?: string[]; + provenances?: string[]; + }; + page?: number; + }; +}; + describe("find-users page", () => { let mockRequest: Partial<Request>; let mockResponse: Partial<Response>; - let mockSession: any; + let mockSession: MockUserManagementSession;As per coding guidelines: Use TypeScript strict mode. Avoid
anywithout justification.libs/system-admin-pages/src/pages/find-users/clear-filters/index.ts (1)
1-15: MoveUserManagementSessionbelow handlers to match module ordering.This keeps interfaces/types at the bottom as required.
Proposed refactor
import { requireRole, USER_ROLES } from "@hmcts/auth"; import type { Request, RequestHandler, Response } from "express"; - -interface UserManagementSession { - userManagement?: { - filters?: { - email?: string; - userId?: string; - userProvenanceId?: string; - roles?: string[]; - provenances?: string[]; - }; - page?: number; - }; -} const getHandler = async (req: Request, res: Response) => { const language = req.query.lng === "cy" ? "cy" : "en"; const session = req.session as UserManagementSession; @@ export const GET: RequestHandler[] = [requireRole([USER_ROLES.SYSTEM_ADMIN]), getHandler]; + +interface UserManagementSession { + userManagement?: { + filters?: { + email?: string; + userId?: string; + userProvenanceId?: string; + roles?: string[]; + provenances?: string[]; + }; + page?: number; + }; +}As per coding guidelines: Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom.
libs/system-admin-pages/src/pages/find-users/remove-filter/index.ts (1)
1-15: MoveUserManagementSessionbelow handlers to match module ordering.This aligns type placement with the project’s ordering rules.
Proposed refactor
import { requireRole, USER_ROLES } from "@hmcts/auth"; import type { Request, RequestHandler, Response } from "express"; - -interface UserManagementSession { - userManagement?: { - filters?: { - email?: string; - userId?: string; - userProvenanceId?: string; - roles?: string[]; - provenances?: string[]; - }; - page?: number; - }; -} const getHandler = async (req: Request, res: Response) => { @@ export const GET: RequestHandler[] = [requireRole([USER_ROLES.SYSTEM_ADMIN]), getHandler]; + +interface UserManagementSession { + userManagement?: { + filters?: { + email?: string; + userId?: string; + userProvenanceId?: string; + roles?: string[]; + provenances?: string[]; + }; + page?: number; + }; +}As per coding guidelines: Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom.
libs/system-admin-pages/src/pages/find-users/index.ts (2)
8-19: MoveUserManagementSessionbelow handlers to match module ordering.This keeps interfaces/types at the bottom as required.
Proposed refactor
import { cy } from "./cy.js"; import { en } from "./en.js"; - -interface UserManagementSession { - userManagement?: { - filters?: { - email?: string; - userId?: string; - userProvenanceId?: string; - roles?: string[]; - provenances?: string[]; - }; - page?: number; - }; -} const getHandler = async (req: Request, res: Response) => { @@ export const GET: RequestHandler[] = [requireRole([USER_ROLES.SYSTEM_ADMIN]), getHandler]; export const POST: RequestHandler[] = [requireRole([USER_ROLES.SYSTEM_ADMIN]), postHandler]; + +interface UserManagementSession { + userManagement?: { + filters?: { + email?: string; + userId?: string; + userProvenanceId?: string; + roles?: string[]; + provenances?: string[]; + }; + page?: number; + }; +}As per coding guidelines: Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom.
60-61: Add explicit types forselectedFilterGroups/paginationItems.Empty array literals introduce implicit
any[]under strict mode; explicit types keep the handler safe and self-documenting.Proposed refactor
- const selectedFilterGroups = []; + const selectedFilterGroups: Array<{ + heading: string; + tags: Array<{ label: string; removeUrl: string }>; + }> = []; @@ - const paginationItems = []; + const paginationItems: Array<{ + number: number; + href: string; + current?: boolean; + visuallyHiddenText?: string; + }> = [];As per coding guidelines: Use TypeScript strict mode. Avoid
anywithout justification.Also applies to: 139-140
| const userRows = searchResult.users.map((user) => [ | ||
| { text: user.email }, | ||
| { text: user.role }, | ||
| { text: user.userProvenance }, | ||
| { html: `<a href="/manage-user/${user.userId}${lngParam}" class="govuk-link">${content.manageLink}</a>` } | ||
| ]); |
There was a problem hiding this comment.
User rows omit required fields and Welsh labels.
The list only outputs email/role/provenance/manage and uses raw enum values. The PR objectives require User ID, Provenance ID, Created, and Last sign-in columns, and role/provenance values should be localised (especially for lng=cy). Please extend userRows and reuse the role/provenance label mapping for i18n.
Changed expected property from 'users' to 'userRows' to match actual render call. The controller maps users to rows before rendering, so the test should check for userRows instead. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed toHaveClass check for .govuk-heading-l as E2E tests should not test visual styling. The test already verifies the heading is visible and contains the correct text, which are the functional requirements. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed test that checks for 2-column grid structure as it tests visual styling/layout rather than user functionality. E2E tests should focus on user behavior and content, not UI design details. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Refactored system-admin-dashboard E2E tests to follow best practices: - Combined 10+ separate tests into 1 comprehensive journey test - Inlined accessibility checks (WCAG 2.2 AA) within the main journey - Included keyboard navigation testing in the journey flow - Added tile click navigation to verify complete user flow - Removed visual styling tests (CSS classes, grid layout, hover states) - Kept responsive design tests as @nightly (different viewports) The single journey test now covers: - Page load and title verification - Content verification (heading, 8 tiles with titles/links/descriptions) - Accessibility validation inline - Keyboard navigation functionality - Complete navigation flow by clicking a tile This follows E2E testing guidelines: minimize test count with one test per journey, include accessibility/validation inline rather than separate. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed 3 viewport-specific tests (mobile, tablet, desktop) as they test visual layout/responsiveness rather than user functionality. E2E tests should focus on user journeys and behavior, not visual presentation at different screen sizes. Responsive design is the responsibility of CSS and the GOV.UK Design System. Test count reduced from 4 to 1 comprehensive journey test. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replaced CSS selectors with accessibility-focused getByRole() selectors:
- Changed h1 locators to getByRole("heading", { level: 1 })
- Changed link selectors from CSS (.admin-tile, a:has-text) to getByRole("link")
- Removed redundant tile count check
- Simplified description checks by verifying content within link elements
- Updated keyboard navigation to use getByRole()
getByRole() is more resilient to implementation changes and follows
accessibility best practices. Selector priority: getByRole() > getByLabel()
> getByText() > getByTestId().
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replaced `any` type for mockUser with proper MockUser interface. This improves type safety and makes the test more self-documenting. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The previous logic checked Object.keys(filters).length which is always greater than 0 since the filters object always has keys (email, userId, etc.), even when values are undefined. This caused the no-results error to appear even when no filters were actually applied. Changed to check if any filter has an active value: - For arrays: check if length > 0 - For other values: check if truthy Added tests to verify: - No error shown when all filters are undefined - No error shown when array filters are empty - Error still shown when active filters return no results Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Applied the same role and provenance label mappings to the table display that were already being used for filter tags. This ensures: - Roles display as "System Admin" instead of "SYSTEM_ADMIN" - Provenances display as "CFT IdAM" instead of "CFT_IDAM" - All labels are properly localized for Welsh users The table keeps the simple 4-column layout (Email, Role, Provenance, Manage) without additional columns for User ID, Provenance ID, Created, or Last Sign-in. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Updated test expectations to match the localized labels: - "VERIFIED" -> "Verified" - "CFT_IDAM" -> "CFT IdAM" Added test to verify Welsh localization works correctly: - "SYSTEM_ADMIN" -> "Gweinyddwr System" when lng=cy Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The GOV.UK pagination component expects previous/next as separate
top-level properties, not as items in the paginationItems array.
Changes:
- Separated previous and next into paginationPrevious and paginationNext
- paginationItems now only contains numbered page objects
- paginationPrevious/Next are undefined when not applicable (first/last page)
- Updated template to pass all three properties to govukPagination
- Updated tests to verify correct structure
- Added tests for edge cases (first page, last page)
This follows the GOV.UK Design System specification where:
- previous: { href: "..." } (optional)
- next: { href: "..." } (optional)
- items: [{ number: 1, href: "...", current: false }, ...]
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| const page = Number(req.query.page) || session.userManagement?.page || 1; | ||
|
|
There was a problem hiding this comment.
Normalise page to a positive integer.
Number(req.query.page) can be negative, fractional, or NaN; negative pages lead to invalid pagination/skips. Clamp to a positive integer before calling searchUsers.
✅ Proposed fix
- const page = Number(req.query.page) || session.userManagement?.page || 1;
+ const pageParam = Number(req.query.page);
+ const page =
+ Number.isFinite(pageParam) && pageParam > 0
+ ? Math.floor(pageParam)
+ : session.userManagement?.page || 1;Added 50 test users with varied roles and provenances: - Roles: VERIFIED, INTERNAL_ADMIN_CTSC, INTERNAL_ADMIN_LOCAL, SYSTEM_ADMIN - Provenances: CFT_IDAM, SSO, B2C, CRIME_IDAM - Staggered creation dates (1 day apart) - Some users have never signed in (lastSignedInDate = null) - Fixed UUIDs: 10000000-0000-0000-0000-000000000001 to 000000000050 With 25 users per page, this creates 2 pages for testing pagination. Run: cd apps/postgres && npx tsx prisma/seed.ts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/postgres/prisma/seed.ts (1)
93-104: Consider batching database operations.Sequential
findUniqueandcreatecalls result in 100 round-trips. For seed scripts,createManywithskipDuplicatesor a transaction-wrapped approach would be more efficient, though this is acceptable for development tooling.Alternative using createMany
- for (const user of testUsers) { - const existing = await prisma.user.findUnique({ - where: { userId: user.userId } - }); - - if (!existing) { - await prisma.user.create({ data: user }); - console.log(`Created test user: ${user.email}`); - } else { - console.log(`User ${user.email} already exists`); - } - } + const result = await prisma.user.createMany({ + data: testUsers, + skipDuplicates: true + }); + console.log(`Created ${result.count} test users (${testUsers.length - result.count} already existed).`);
Fixed delete-user-success template using correct Nunjucks if syntax
instead of ternary operator which caused template render errors.
Changed from: {{ lng ? '?lng=' + lng : '' }}
To: {% if lng %}?lng={{ lng }}{% endif %}
Also removed debug console.log from find-users controller.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added proper cascade deletion to handle foreign key constraints: 1. Find all subscriptions for the user 2. Delete NotificationAuditLog records linked to those subscriptions 3. Delete the subscriptions 4. Delete the user This prevents foreign key constraint violations since NotificationAuditLog references Subscription (via subscriptionId) without cascade delete configured. Added tests: - Verify audit logs are deleted before subscriptions - Verify audit log deletion is skipped when user has no subscriptions - Verify correct order of operations in transaction Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/system-admin-pages/src/user-management/queries.ts (1)
4-31: Align module ordering with the project guidelines.Constants should be at the top and interfaces/types at the bottom. Please reorder
PAGE_SIZEabove exports and move the interfaces to the bottom of the file.♻️ Suggested re-ordering
import { prisma } from "@hmcts/postgres"; import type { Prisma } from "@prisma/client"; - interface UserSearchFilters { - email?: string; - userId?: string; - userProvenanceId?: string; - roles?: string[]; - provenances?: string[]; - } - - interface UserSearchResult { - users: User[]; - totalCount: number; - currentPage: number; - totalPages: number; - } - - interface User { - userId: string; - email: string; - firstName: string | null; - surname: string | null; - userProvenance: string; - userProvenanceId: string; - role: string; - createdDate: Date; - lastSignedInDate: Date | null; - } - const PAGE_SIZE = 25; export async function searchUsers(filters: UserSearchFilters, page: number = 1): Promise<UserSearchResult> { ... } export async function getUserById(userId: string): Promise<User | null> { ... } export async function deleteUserById(userId: string): Promise<void> { ... } + +interface UserSearchFilters { + email?: string; + userId?: string; + userProvenanceId?: string; + roles?: string[]; + provenances?: string[]; +} + +interface UserSearchResult { + users: User[]; + totalCount: number; + currentPage: number; + totalPages: number; +} + +interface User { + userId: string; + email: string; + firstName: string | null; + surname: string | null; + userProvenance: string; + userProvenanceId: string; + role: string; + createdDate: Date; + lastSignedInDate: Date | null; +}
| export async function searchUsers(filters: UserSearchFilters, page: number = 1): Promise<UserSearchResult> { | ||
| const skip = (page - 1) * PAGE_SIZE; | ||
|
|
There was a problem hiding this comment.
Guard against page <= 0 to avoid negative skip.
If a caller passes 0/negative, Prisma will throw due to a negative skip. Consider clamping or throwing early.
✅ Safer pagination normalisation
-export async function searchUsers(filters: UserSearchFilters, page: number = 1): Promise<UserSearchResult> {
- const skip = (page - 1) * PAGE_SIZE;
+export async function searchUsers(filters: UserSearchFilters, page: number = 1): Promise<UserSearchResult> {
+ const safePage = Math.max(1, page);
+ const skip = (safePage - 1) * PAGE_SIZE;
...
return {
users,
totalCount,
- currentPage: page,
+ currentPage: safePage,
totalPages
};
}Line 39 was injecting user-supplied filter labels (email, userId,
userProvenanceId) directly into HTML without escaping. This created
an XSS attack vector where malicious input like:
email: <script>alert('XSS')</script>@example.com
Would execute arbitrary JavaScript when displayed as a filter tag.
Fixed by using Nunjucks' escape filter: (tag.label | escape)
This ensures all user input is HTML-encoded before rendering.
Line 213 was calling searchUsers({}, 1) without try-catch during
validation error handling. If the database query failed, the request
would crash instead of gracefully handling the error.
Now wraps the searchUsers call in try-catch with:
- Error logging with context (filters, timestamp)
- Fallback to empty result set
- Consistent error handling pattern with GET handler
This ensures users still see validation errors even if the
database query fails.
Moved validateEnvVars() call from line 28 to line 33, inside the SSO redirect check. This prevents test failures in local/non-SSO environments where SSO credentials aren't required. Before: - validateEnvVars() called before navigation - Failed immediately if env vars missing, even for local dev After: - validateEnvVars() only called when SSO redirect detected - Allows tests to run locally without SSO credentials - Only validates when SSO is actually needed
Line 106 was logging 'Created 50 test users' even when some were skipped due to existing records, providing inaccurate reporting. Changes: - Added createdCount and skippedCount tracking - Increment counters based on actual create/skip operations - Updated final log message to show both counts Example output: Before: 'Seed completed successfully! Created 50 test users.' After: 'Seed completed successfully! Created 3 test users, skipped 47 existing users.' This provides accurate feedback on what the seed script actually did.
- Add type annotation for searchResult variable in POST handler - Fix import order in user-management E2E test - Apply biome formatting (line length adjustments) - Use template literal instead of string concatenation All files now pass biome lint checks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e-tests/tests/user-management.spec.ts (2)
210-285: Consider folding the extra@nightlytests into the main journey.
The first test already covers a full end‑to‑end flow (incl. Welsh + a11y). Keeping the remaining scenarios in separate tests increases count against the single‑journey guidance; consider merging them unless isolation is required.Based on learnings E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests.
106-116: Prefer role/label locators over CSS selectors for form controls.
The CSS selectors for checkboxes/radios are brittle and bypass accessible names;getByRole/getByLabelkeeps tests resilient and aligned to a11y.Suggested locator updates
- await page.locator('input[name="roles"][value="VERIFIED"]').check(); - await page.locator('input[name="provenances"][value="CFT_IDAM"]').check(); + await page.getByRole("checkbox", { name: "Verified" }).check(); + await page.getByRole("checkbox", { name: "CFT IdAM" }).check(); ... - await page.locator('input[name="confirmation"][value="yes"]').check(); + await page.getByRole("radio", { name: "Yes" }).check(); ... - await page.locator('input[name="confirmation"][value="no"]').check(); + await page.getByRole("radio", { name: "No" }).check();Based on learnings E2E tests should tag nightly-only tests with nightly in the test title and use getByRole() first, getByLabel() for inputs, getByText() for text, and getByTestId() as last resort.
Also applies to: 192-194, 229-231
| test("system admin can filter by user ID @nightly", async ({ page }) => { | ||
| // Create test user | ||
| const testEmail = `userid.test.${Date.now()}@example.com`; | ||
| const testUserId = await createTestUser(testEmail, "SYSTEM_ADMIN", "CRIME_IDAM"); | ||
|
|
||
| // Authenticate as system admin | ||
| await authenticateSystemAdmin(page); | ||
|
|
||
| // Navigate to find users | ||
| await page.getByRole("link", { name: "User Management" }).click(); | ||
|
|
||
| // Note: User ID filter has validation that requires alphanumeric only, | ||
| // but UUIDs contain hyphens, so this test verifies that results are shown | ||
| // Search by email instead to find the specific user | ||
| await page.getByLabel("Email").fill(testEmail); | ||
| await page.getByRole("button", { name: "Apply filters" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Verify the specific user is found | ||
| await expect(page.getByText(/1 users? found/)).toBeVisible(); | ||
|
|
||
| // Verify we can access the user details | ||
| await page.getByRole("link", { name: "Manage" }).first().click(); | ||
| await expect(page.getByRole("heading", { name: `Manage ${testEmail}`, level: 1 })).toBeVisible(); | ||
| await expect(page.getByText(testUserId)).toBeVisible(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the test
fd "user-management.spec.ts" e2e-testsRepository: hmcts/cath-service
Length of output: 101
🏁 Script executed:
# Read the specific test file and lines around 244-268
cat -n e2e-tests/tests/user-management.spec.ts | sed -n '240,275p'Repository: hmcts/cath-service
Length of output: 1994
🏁 Script executed:
# Search for "User ID" field usage in the e2e tests
rg "User ID" e2e-tests --context 2Repository: hmcts/cath-service
Length of output: 1062
🏁 Script executed:
# Check if there are other tests in the same file that use filters
rg "getByLabel" e2e-tests/tests/user-management.spec.ts --context 1Repository: hmcts/cath-service
Length of output: 1150
Rename test to match what it actually exercises.
The test is titled "filter by user ID" but filters by email. The code comment correctly explains why: the User ID field validates alphanumeric input only and rejects UUIDs containing hyphens. Renaming the test to "system admin can filter by email" or similar would align the title with the actual user journey being tested.
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
libs/verified-pages/src/pages/subscription-locations-review/index.njk-32-39 (1)
32-39:⚠️ Potential issue | 🟡 MinorRemove links lack unique accessible names for screen readers.
Every row renders identical link text (
removeLink= "Remove"), making it impossible for assistive technology users to distinguish which location each link targets. Add visually hidden text with the location name.♿ Proposed fix
<td class="govuk-table__cell"> - <a href="/subscription-locations-review?remove={{ location.locationId }}" class="govuk-link">{{ removeLink }}</a> + <a href="/subscription-locations-review?remove={{ location.locationId }}" class="govuk-link"> + {{ removeLink }}<span class="govuk-visually-hidden"> {{ location.name }}</span> + </a> </td>e2e-tests/tests/user-management.spec.ts-40-54 (1)
40-54:⚠️ Potential issue | 🟡 Minor
substris deprecated; usesubstringinstead.
String.prototype.substris deprecated. Replace withsubstring.Proposed fix
- userProvenanceId: `test-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, + userProvenanceId: `test-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`,e2e-tests/tests/user-management.spec.ts-204-204 (1)
204-204:⚠️ Potential issue | 🟡 MinorChange
"B2C"to"B2C_IDAM"at line 204. The TypeScript model and codebase define four valid provenance values:"SSO","CFT_IDAM","CRIME_IDAM", and"B2C_IDAM". Line 204 uses the invalid value"B2C"; line 238 correctly uses"CRIME_IDAM".libs/verified-pages/src/pages/subscription-list-language/index.test.ts-139-156 (1)
139-156:⚠️ Potential issue | 🟡 MinorNote: Welsh error text may be incorrect (see
cy.tsreview).The assertion at line 150 matches the potentially malformed
errorRequiredstring fromcy.ts. If that string is corrected, this assertion will need updating too.libs/verified-pages/src/pages/subscription-list-language/cy.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorPossibly malformed Welsh error message.
errorRequiredappears to concatenate two phrases without punctuation: roughly "Select a version of the list type" + "Select an option". This reads like a copy-paste artefact. Compare with the English equivalent which would be a single coherent sentence. Please verify the intended Welsh translation.libs/verified-pages/src/pages/delete-list-type-subscription/index.ts-16-20 (1)
16-20:⚠️ Potential issue | 🟡 MinorSilent failure on deletion error — user receives no feedback.
When
deleteListTypeSubscriptionthrows, the error is logged but the user is silently redirected back to/subscription-managementwith no indication that the deletion failed. Consider passing an error state via session/flash so the management page can display a notification.libs/verified-pages/src/pages/subscription-list-language/en.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorError message style inconsistency and missing article.
"Please select version of the list type to continue"— missing "a" before "version", and uses "Please" which differs from other error messages in the codebase (e.g.,subscription-add-method/en.tsuses the imperative"Select how you want to..."without "Please"), aligning with GOV.UK error message conventions.Suggested wording
- errorRequired: "Please select version of the list type to continue", + errorRequired: "Select a version of the list type to continue",libs/verified-pages/src/pages/subscription-confirm/index.njk-61-74 (1)
61-74:⚠️ Potential issue | 🟡 MinorEmpty table rendered when there are no list types.
When
hasNoListTypesis true, thegovukTableat lines 61–68 still renders (with presumably emptylistTypeRows), showing table headers with no content. Consider wrapping the table in a conditional or showing a "no list types selected" message instead.Proposed fix
+{% if not hasNoListTypes %} {{ govukTable({ head: [ { text: listTypesHeading }, { text: actionsHeading, classes: "govuk-table__header--numeric" } ], rows: listTypeRows, classes: "subscription-confirm-table" }) }} +{% endif %} {% if hasNoListTypes %} <p class="govuk-body"> <a href="/subscription-list-types" class="govuk-link">{{ selectListTypesLink }}</a> </p> {% endif %}libs/verified-pages/src/pages/subscription-management/index.njk-82-83 (1)
82-83:⚠️ Potential issue | 🟡 MinorHardcoded English
aria-labelbreaks bilingual accessibility.The
aria-labelon line 82 is always in English. Welsh-language users will hear English text from their screen reader. Consider using a translatable string variable instead.Proposed fix
- <button type="submit" class="govuk-link govuk-button-as-link" aria-label="Remove subscription for {{ subscription.listTypeName }}"> + <button type="submit" class="govuk-link govuk-button-as-link" aria-label="{{ removeListTypeAriaLabel }} {{ subscription.listTypeName }}">libs/verified-pages/src/pages/subscription-by-location/index.njk-134-160 (1)
134-160:⚠️ Potential issue | 🟡 MinorTable missing
<thead>and column headers — accessibility concern.The acceptance criteria require WCAG 2.2 AA compliance with semantic table markup. This table has no
<thead>or<th>elements, which means screen readers cannot associate cells with column headers.Proposed fix
<table class="govuk-table court-table"> + <thead class="govuk-table__head govuk-visually-hidden"> + <tr class="govuk-table__row"> + <th class="govuk-table__header" scope="col">{{ letterHeading or "Letter" }}</th> + <th class="govuk-table__header" scope="col">{{ courtNameHeading or "Court or tribunal name" }}</th> + </tr> + </thead> <tbody class="govuk-table__body">libs/verified-pages/src/pages/subscription-list-types/index.njk-39-51 (1)
39-51:⚠️ Potential issue | 🟡 MinorDuplicate
<label>elements for the same input may confuse assistive technology.Each checkbox has two
<label>elements withfor="listTypes-{{ item.value }}": one visually hidden (Line 43–44) and one visible (Line 50). While browsers associate the firstfor-matched label, multiple labels for one input can produce inconsistent screen reader announcements. Consider removing the hidden label and keeping only the visible one, or usingaria-labelledbyinstead.libs/verified-pages/src/pages/subscription-list-types/index.njk-31-56 (1)
31-56:⚠️ Potential issue | 🟡 MinorTable lacks
<thead>and column headers.For WCAG 2.2 AA compliance (a stated PR objective), data tables should have header cells (
<th>) so that screen readers can associate data with column meanings. Adding a visually-hidden header row would satisfy this.libs/verified-pages/src/pages/subscription-list-language/index.ts-37-48 (1)
37-48:⚠️ Potential issue | 🟡 MinorNo validation of the
languagevalue.The POST handler checks that
languageis truthy but doesn't validate it against the expected set (e.g.,"ENGLISH","WELSH","BOTH"). Any arbitrary string from the request body will be persisted to the session.Suggested fix
+ const VALID_LANGUAGES = ["ENGLISH", "WELSH", "BOTH"]; + if (!language) { ... } + + if (!VALID_LANGUAGES.includes(language)) { + // treat as validation error, same as missing + ... + }libs/verified-pages/src/pages/subscription-confirm/index.ts-173-183 (1)
173-183:⚠️ Potential issue | 🟡 MinorDead code:
hasNoListTypescheck on line 180 is unreachable.Line 174 already redirects when
selectedListTypeIds.length === 0. The subsequenthasNoListTypescheck at line 180 (which tests the same condition) can never be true. Simplify to only checkhasNoLocations.♻️ Proposed fix
if (req.session.listTypeSubscription.selectedListTypeIds.length === 0) { return res.redirect("/subscription-list-types"); } - // Check if locations or list types are empty (no subscriptions) - const hasNoLocations = !req.session.listTypeSubscription.selectedLocationIds || req.session.listTypeSubscription.selectedLocationIds.length === 0; - const hasNoListTypes = req.session.listTypeSubscription.selectedListTypeIds.length === 0; - if (hasNoLocations || hasNoListTypes) { + const hasNoLocations = !req.session.listTypeSubscription.selectedLocationIds || req.session.listTypeSubscription.selectedLocationIds.length === 0; + if (hasNoLocations) { return res.redirect("/subscription-add-method"); }
🧹 Nitpick comments (28)
libs/web-core/src/assets/css/filter-panel.scss (1)
140-164: Consider using SCSS variables for repeated colour literals.Colours like
#1d70b8,#003078, and#0b0c0cappear multiple times across this file. Extracting them into variables (e.g.,$govuk-link-colour) would reduce duplication and ease future updates.libs/verified-pages/src/pages/subscription-locations-review/index.ts (1)
47-55: Sequential awaits — considerPromise.allfor parallel fetching.Each
getLocationByIdcall is awaited in series. If a user has selected many locations, this adds up linearly.♻️ Proposed parallel fetch
- const locationRows: LocationRow[] = []; - for (const locationId of selectedLocationIds) { - const location = await getLocationById(locationId); - if (location) { - locationRows.push({ - locationId: location.locationId, - name: locale === "cy" ? location.welshName : location.name - }); - } - } + const locations = await Promise.all(selectedLocationIds.map((id) => getLocationById(id))); + const locationRows: LocationRow[] = locations + .filter((location) => location !== null && location !== undefined) + .map((location) => ({ + locationId: location.locationId, + name: locale === "cy" ? location.welshName : location.name + }));docs/tickets/296/review.md (1)
1-459: Consider whether this AI-generated review document belongs in the repository.Committing a lengthy AI-generated code review as a versioned file is unusual. This type of artefact is typically ephemeral (PR comments, wiki pages) rather than part of the source tree. It will grow stale quickly and isn't maintained alongside the code it describes. Consider moving it to a wiki, PR comment, or ADR format if the intent is to preserve the decision record.
e2e-tests/tests/user-management.spec.ts (1)
107-111: PrefergetByRole/getByLabelover CSS selectors for checkboxes and radios.Per coding guidelines, E2E tests should use
getByRole()first,getByLabel()for inputs, andgetByTestId()as a last resort. The CSS locators on Lines 109–110 (and similarly Lines 184, 221) could be replaced withgetByLabelif the checkbox/radio labels are unique. If the labels aren't sufficiently distinct, this is acceptable as-is.libs/subscription-list-types/src/locales/cy.ts (1)
1-6: Minor naming inconsistency:backLinkvsbackLinkText.This shared locale uses
backLink(Line 3), but the page-specific locale atlibs/verified-pages/src/pages/subscription-list-types/cy.tsusesbackLinkTextfor the same Welsh string"Yn ôl". Consider aligning the key names across locales to avoid confusion when composing content objects.libs/verified-pages/src/pages/subscription-list-types/en.ts (1)
1-15: GOV.UK error message style convention.Lines 12 and 14 use "Please select…" phrasing. GOV.UK Design System guidance typically recommends direct imperative phrasing without "Please" (e.g., "Select a list type to continue"). This is a minor style point — the Welsh counterpart in
cy.tsalready uses the direct imperative form ("Dewiswch...").libs/verified-pages/src/pages/subscription-add-method/index.test.ts (2)
15-25: Consider addingsessiontomockReq.If the handler (or a future change) reads from
req.session(e.g., to restore previous selection or store list-type subscription state), the missingsessionproperty will cause a runtime error in tests. Worth adding an empty object now for resilience.Proposed change
mockReq = { body: {}, - path: "/subscription-add-method" + path: "/subscription-add-method", + session: {} as any };
58-81: No test for an unrecognisedsubscriptionMethodvalue.The "case" and "reference" options both redirect to
/subscription-management. Consider adding a test with an unexpected value (e.g.,"invalid") to document the handler's fallback behaviour.apps/postgres/prisma/migrations/20260205161949_add_subscription_list_type/migration.sql (1)
13-19: Redundant indexidx_subscription_list_type_user.The unique composite index on
(user_id, list_type_id, language)at line 19 already hasuser_idas its leftmost column, so PostgreSQL can use it for queries filtering byuser_idalone. The standalone index at line 13 is redundant and adds write overhead without benefit.🔧 Proposed fix
--- Remove the redundant single-column index -CREATE INDEX "idx_subscription_list_type_user" ON "subscription_list_type"("user_id"); -libs/verified-pages/src/pages/subscription-by-location/index.test.ts (1)
40-56: No Welsh locale test for GET handler.Other page tests in this PR (e.g.
subscription-list-language/index.test.ts) verify Welsh content whenlocaleiscy. This test only covers the English path. As per coding guidelines, every page must support both English and Welsh, and templates should test with?lng=cyquery parameter.libs/verified-pages/src/pages/subscription-confirm/index.njk (1)
15-32: Inline styles should be in a separate stylesheet.Embedding
<style>blocks inside templates makes styles harder to maintain and cache. Consider moving these to a co-located.scssfile.libs/verified-pages/src/pages/delete-list-type-subscription/index.ts (1)
19-19: Use structured logger instead ofconsole.error.The PR objectives mention enhanced structured error logging. Using
console.errorhere is inconsistent with that goal. If a shared logger exists in the project, prefer that.#!/bin/bash # Check if there's a structured logger used elsewhere rg -n "import.*logger" --type=ts -l | head -10 rg -n "from.*@hmcts.*log" --type=ts | head -10libs/verified-pages/src/pages/subscription-by-location/index.njk (1)
27-35: Prefer CSS classes over inline styles.Lines 27, 30, and 35 use inline
styleattributes for borders and background colours. These should live in a stylesheet (e.g., alongsideuser-management.scss) for maintainability and stricter CSP policies.libs/verified-pages/src/pages/subscription-management/index.ts (1)
35-48: Language display map is duplicated across files.The
languageDisplaymapping (ENGLISH/WELSH/BOTH → localised strings) appears here and is likely repeated in other pages (e.g.,subscription-confirm). Consider extracting it into the shared locale files or a small utility.libs/verified-pages/src/pages/subscription-add-method/index.ts (2)
44-56: Redundant redirect branches.
"case","reference", and the fallback all redirect to/subscription-management. This could be a singleif/else.Suggested simplification
- if (subscriptionMethod === "court") { - return res.redirect("/subscription-by-location"); - } - - if (subscriptionMethod === "case") { - return res.redirect("/subscription-management"); - } - - if (subscriptionMethod === "reference") { - return res.redirect("/subscription-management"); - } - - return res.redirect("/subscription-management"); + if (subscriptionMethod === "court") { + return res.redirect("/subscription-by-location"); + } + + return res.redirect("/subscription-management");
11-13: Guard arounddeleteis unnecessary.
delete req.session.listTypeSubscriptionis safe even when the property doesn't exist — theifcheck on Line 11 is redundant.libs/verified-pages/src/pages/subscription-list-language/index.ts (1)
58-58: Use structured logger instead ofconsole.error.The PR objectives mention enhanced structured error logging. Replace
console.errorwith the project's structured logger for consistency and observability.libs/verified-pages/src/pages/subscription-list-types/index.ts (1)
134-134: Use structured logger instead ofconsole.error.Same as in
subscription-list-language/index.ts— replace with the project's logger.libs/verified-pages/src/pages/subscription-list-types/index.njk (1)
36-49: Heavy use of inline styles.Multiple cells use inline
styleattributes for width, alignment, and sizing. Prefer dedicated CSS classes (or GOV.UK utility classes) for maintainability and consistency.e2e-tests/tests/subscription-list-types.spec.ts (3)
190-217: Third test duplicates validations already covered in the main journey test.Test 1 already exercises the same validation errors for subscription-add-method (line 22–24), subscription-list-types (line 45–47), and subscription-list-language (line 76–78). This test adds no new coverage beyond what's already inline. Consider removing it or folding any unique assertions into the primary journey test. As per coding guidelines, E2E tests should "minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests."
83-87: Brittle keyboard navigation — hardcoded Tab count.Two
Tabpresses assumes a specific DOM order between the "English" radio and the "Continue" button. If the page gains or loses a focusable element, this silently tests the wrong thing. Consider locating the button and calling.focus()directly, or usingpage.keyboard.press("Tab")in a loop until the expected element is focused.
4-9: Repeated sign-in block across all three tests.The same sign-in sequence (goto, fill email, fill password, click) is copy-pasted in each test. The imports section references
e2e-tests/utils/sso-helpers.ts— consider extracting a shared helper to reduce duplication.Also applies to: 148-153, 190-195
libs/verified-pages/src/pages/subscription-by-location/index.ts (2)
209-209: Unsafeanycast for CSRF token.
(req as any).csrfToken?.()bypasses type safety. Consider extending the ExpressRequesttype or using a typed middleware interface so the CSRF token accessor is properly typed.
43-59: Triplicated query-param-to-number-array parsing.The same pattern (check
Array.isArray, map toNumber, filterisFinite, else single-value) is repeated forjurisdictionParam,regionParam, andsubJurisdictionParam. Extract a small helper liketoNumberArray(param)to reduce duplication.♻️ Example helper
+const toNumberArray = (param: unknown): number[] => { + if (Array.isArray(param)) { + return param.map(Number).filter(Number.isFinite); + } + if (param && Number.isFinite(Number(param))) { + return [Number(param)]; + } + return []; +}; + const getHandler = async (req: Request, res: Response) => { ... - const selectedJurisdictions = Array.isArray(jurisdictionParam) - ? jurisdictionParam.map(Number).filter(Number.isFinite) - : jurisdictionParam && Number.isFinite(Number(jurisdictionParam)) - ? [Number(jurisdictionParam)] - : []; + const selectedJurisdictions = toNumberArray(jurisdictionParam); + const selectedRegions = toNumberArray(regionParam); + const selectedSubJurisdictions = toNumberArray(subJurisdictionParam);libs/system-admin-pages/src/pages/find-users/index.ts (2)
99-105:roleLabelsandprovenanceLabelsmaps defined twice.The same mappings appear at lines 100–105 and 134–139 (roles) and lines 117–122 and 141–146 (provenances). Hoist them above both usages to avoid duplication.
Also applies to: 134-139
158-165: Unbounded pagination items for large result sets.Every page number from 1 to
totalPagesis emitted. With many users, this could generate hundreds of pagination links. GOV.UK pagination pattern recommends using ellipsis to truncate. Consider capping visible page numbers and inserting{ ellipsis: true }entries.libs/subscription-list-types/src/subscription-list-type/service.ts (1)
35-49: Redundant find-before-delete indeleteListTypeSubscription.
findListTypeSubscriptionById(line 36) fetches the record only to confirm it exists, thendeleteListTypeSubscriptionRecord(line 42) deletes by the same criteria and independently checkscount === 0. The initial find adds a round-trip without value. Deleting directly and checking the returned count is sufficient.♻️ Simplified
export async function deleteListTypeSubscription(userId: string, listTypeSubscriptionId: string) { - const subscription = await findListTypeSubscriptionById(listTypeSubscriptionId, userId); - - if (!subscription) { - throw new Error("Subscription not found"); - } - const count = await deleteListTypeSubscriptionRecord(listTypeSubscriptionId, userId); if (count === 0) { throw new Error("Subscription not found"); } return count; }libs/verified-pages/src/pages/subscription-confirm/index.ts (1)
154-162: Fragile string-matching for error classification.
getUserFriendlyErrorMessagerelies on substrings like"Maximum","50", and"Already subscribed"in the error message. If the wording inservice.tschanges, the mapping silently falls through toerrorGeneric. Consider using typed error subclasses or error codes instead.♻️ Sketch using error codes
// In service.ts export class MaxSubscriptionsError extends Error { code = "MAX_SUBSCRIPTIONS" as const; } export class DuplicateSubscriptionError extends Error { code = "DUPLICATE_SUBSCRIPTION" as const; } // In subscription-confirm/index.ts const getUserFriendlyErrorMessage = (error: Error, t: typeof en | typeof cy) => { if ("code" in error && error.code === "MAX_SUBSCRIPTIONS") return t.errorMaxSubscriptions; if ("code" in error && error.code === "DUPLICATE_SUBSCRIPTION") return t.errorDuplicateSubscription; return t.errorGeneric; };
| export async function createListTypeSubscriptions(userId: string, listTypeIds: number[], language: string) { | ||
| const currentCount = await countListTypeSubscriptionsByUserId(userId); | ||
|
|
||
| if (currentCount + listTypeIds.length > MAX_LIST_TYPE_SUBSCRIPTIONS) { | ||
| throw new Error(`Maximum ${MAX_LIST_TYPE_SUBSCRIPTIONS} list type subscriptions allowed`); | ||
| } | ||
|
|
||
| for (const listTypeId of listTypeIds) { | ||
| const duplicate = await findDuplicateListTypeSubscription(userId, listTypeId, language); | ||
| if (duplicate) { | ||
| throw new Error(`Already subscribed to list type ${listTypeId} with language ${language}`); | ||
| } | ||
| } | ||
|
|
||
| const subscriptions = await Promise.all(listTypeIds.map((listTypeId) => createListTypeSubscriptionRecord(userId, listTypeId, language))); | ||
|
|
||
| return subscriptions; |
There was a problem hiding this comment.
createListTypeSubscriptions is not atomic — partial creation and race conditions possible.
- Partial writes:
Promise.all(line 26) fires all inserts concurrently. If one fails mid-way (e.g., DB constraint violation on a duplicate), the others may have already committed, leaving the user with a partial set of subscriptions. - TOCTOU: The count check (line 13) and duplicate checks (lines 19–24) are not atomic with the subsequent creates. A concurrent request for the same user can bypass both guards.
Consider wrapping the entire operation in a Prisma $transaction to ensure all-or-nothing semantics.
🛡️ Sketch
export async function createListTypeSubscriptions(userId: string, listTypeIds: number[], language: string) {
- const currentCount = await countListTypeSubscriptionsByUserId(userId);
-
- if (currentCount + listTypeIds.length > MAX_LIST_TYPE_SUBSCRIPTIONS) {
- throw new Error(`Maximum ${MAX_LIST_TYPE_SUBSCRIPTIONS} list type subscriptions allowed`);
- }
-
- for (const listTypeId of listTypeIds) {
- const duplicate = await findDuplicateListTypeSubscription(userId, listTypeId, language);
- if (duplicate) {
- throw new Error(`Already subscribed to list type ${listTypeId} with language ${language}`);
- }
- }
-
- const subscriptions = await Promise.all(listTypeIds.map((listTypeId) => createListTypeSubscriptionRecord(userId, listTypeId, language)));
-
- return subscriptions;
+ // Use a transaction for atomicity
+ return prisma.$transaction(async (tx) => {
+ const currentCount = await tx.subscriptionListType.count({ where: { userId } });
+ if (currentCount + listTypeIds.length > MAX_LIST_TYPE_SUBSCRIPTIONS) {
+ throw new Error(`Maximum ${MAX_LIST_TYPE_SUBSCRIPTIONS} list type subscriptions allowed`);
+ }
+ const subscriptions = [];
+ for (const listTypeId of listTypeIds) {
+ const duplicate = await tx.subscriptionListType.findFirst({ where: { userId, listTypeId, language } });
+ if (duplicate) {
+ throw new Error(`Already subscribed to list type ${listTypeId} with language ${language}`);
+ }
+ subscriptions.push(await tx.subscriptionListType.create({ data: { userId, listTypeId, language } }));
+ }
+ return subscriptions;
+ });
}| return res.render("find-users/index", { | ||
| ...content, | ||
| users: searchResult.users, | ||
| totalCount: searchResult.totalCount, | ||
| currentPage: 1, | ||
| totalPages: searchResult.totalPages, | ||
| filters, | ||
| errors: validationErrors, | ||
| lng: language === "cy" ? "cy" : "" | ||
| }); |
There was a problem hiding this comment.
POST validation-error render also omits paginationItems, paginationPrevious, paginationNext, selectedFilterGroups, and hasFilters.
The GET render call (lines 181–195) passes a rich view model with pagination and filter groups. The POST error path passes a minimal subset, so the template will have missing/undefined values for pagination controls and filter tags when re-rendering after a validation error.
| <form method="post"> | ||
| {{ govukButton({ | ||
| text: continueButton, | ||
| disabled: hasNoListTypes | ||
| }) }} | ||
| </form> |
There was a problem hiding this comment.
Missing CSRF token in the form.
The POST form lacks a CSRF hidden input, unlike other forms in the codebase (e.g., subscription-management/index.njk which includes <input type="hidden" name="_csrf" value="{{ csrfToken }}">).
Proposed fix
<form method="post">
+ <input type="hidden" name="_csrf" value="{{ csrfToken }}">
{{ govukButton({
text: continueButton,
disabled: hasNoListTypes
}) }}
</form>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <form method="post"> | |
| {{ govukButton({ | |
| text: continueButton, | |
| disabled: hasNoListTypes | |
| }) }} | |
| </form> | |
| <form method="post"> | |
| <input type="hidden" name="_csrf" value="{{ csrfToken }}"> | |
| {{ govukButton({ | |
| text: continueButton, | |
| disabled: hasNoListTypes | |
| }) }} | |
| </form> |
| // Build table rows | ||
| const locationRows = locations.map((location) => [ | ||
| { text: location.name }, | ||
| { | ||
| html: `<a href="/subscription-confirm?removeLocation=${location.locationId}" class="govuk-link">${t.removeLink}<span class="govuk-visually-hidden"> ${location.name}</span></a>`, | ||
| classes: "govuk-table__cell--numeric" | ||
| } | ||
| ]); | ||
|
|
||
| const listTypeRows = selectedListTypes.map((listType) => { | ||
| const listTypeName = locale === "cy" ? listType.welshFriendlyName : listType.englishFriendlyName; | ||
| return [ | ||
| { text: listTypeName }, | ||
| { | ||
| html: `<a href="/subscription-confirm?removeListType=${listType.id}" class="govuk-link">${t.removeLink}<span class="govuk-visually-hidden"> ${listTypeName}</span></a>`, | ||
| classes: "govuk-table__cell--numeric" | ||
| } | ||
| ]; | ||
| }); | ||
|
|
||
| const languageRows = [ | ||
| [ | ||
| { text: languageDisplay[req.session.listTypeSubscription.language as keyof typeof languageDisplay] || "" }, | ||
| { | ||
| html: `<a href="/subscription-list-language" class="govuk-link">${t.changeLink}<span class="govuk-visually-hidden"> ${t.languageHeading}</span></a>`, | ||
| classes: "govuk-table__cell--numeric" | ||
| } | ||
| ] | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
~60 lines of row-building logic duplicated between GET and POST error paths.
locationRows, listTypeRows, languageRows, and languageDisplay are constructed identically in the GET handler (lines 93–127) and the POST error handler (lines 229–263). Extract a shared helper function to build the view model.
Also applies to: 229-263
| <form method="post" novalidate> | ||
| {{ govukRadios({ | ||
| name: "language", | ||
| items: [ | ||
| { | ||
| value: "ENGLISH", | ||
| text: radioOptionEnglish, | ||
| checked: data.language == "ENGLISH" | ||
| }, | ||
| { | ||
| value: "WELSH", | ||
| text: radioOptionWelsh, | ||
| checked: data.language == "WELSH" | ||
| }, | ||
| { | ||
| value: "BOTH", | ||
| text: radioOptionBoth, | ||
| checked: data.language == "BOTH" | ||
| } | ||
| ], | ||
| errorMessage: errors[0] if errors else undefined | ||
| }) }} | ||
|
|
||
| {{ govukButton({ | ||
| text: continueButton | ||
| }) }} | ||
| </form> |
There was a problem hiding this comment.
Missing CSRF token in the POST form.
Other forms in this PR (e.g., subscription-by-location/index.njk line 133) include <input type="hidden" name="_csrf" value="{{ csrfToken }}">. This form does not, which will cause POST requests to fail if CSRF protection middleware is active.
Proposed fix
<form method="post" novalidate>
+ <input type="hidden" name="_csrf" value="{{ csrfToken }}">
{{ govukRadios({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <form method="post" novalidate> | |
| {{ govukRadios({ | |
| name: "language", | |
| items: [ | |
| { | |
| value: "ENGLISH", | |
| text: radioOptionEnglish, | |
| checked: data.language == "ENGLISH" | |
| }, | |
| { | |
| value: "WELSH", | |
| text: radioOptionWelsh, | |
| checked: data.language == "WELSH" | |
| }, | |
| { | |
| value: "BOTH", | |
| text: radioOptionBoth, | |
| checked: data.language == "BOTH" | |
| } | |
| ], | |
| errorMessage: errors[0] if errors else undefined | |
| }) }} | |
| {{ govukButton({ | |
| text: continueButton | |
| }) }} | |
| </form> | |
| <form method="post" novalidate> | |
| <input type="hidden" name="_csrf" value="{{ csrfToken }}"> | |
| {{ govukRadios({ | |
| name: "language", | |
| items: [ | |
| { | |
| value: "ENGLISH", | |
| text: radioOptionEnglish, | |
| checked: data.language == "ENGLISH" | |
| }, | |
| { | |
| value: "WELSH", | |
| text: radioOptionWelsh, | |
| checked: data.language == "WELSH" | |
| }, | |
| { | |
| value: "BOTH", | |
| text: radioOptionBoth, | |
| checked: data.language == "BOTH" | |
| } | |
| ], | |
| errorMessage: errors[0] if errors else undefined | |
| }) }} | |
| {{ govukButton({ | |
| text: continueButton | |
| }) }} | |
| </form> |
| interface ListTypeSubscriptionSession { | ||
| selectedListTypeIds?: number[]; | ||
| language?: string; | ||
| } | ||
|
|
||
| declare module "express-session" { | ||
| interface SessionData { | ||
| listTypeSubscription?: ListTypeSubscriptionSession; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate ListTypeSubscriptionSession interface and session augmentation.
This interface and declare module block are identical to those in libs/verified-pages/src/pages/subscription-list-types/index.ts (lines 7–16). Extract to a shared location (e.g., a types file in the subscription-list-types package) and import from both controllers.
#!/bin/bash
# Verify duplication across the codebase
rg -n "ListTypeSubscriptionSession" --type=ts| {% block backLink %} | ||
| {{ govukBackLink({ | ||
| text: backLinkText, | ||
| href: "/location-name-search" | ||
| }) }} | ||
| {% endblock %} |
There was a problem hiding this comment.
Back link points to /location-name-search — likely incorrect.
This page is for list-type subscriptions, not location-based ones. The back link should probably navigate to /subscription-add-method (or the previous step in the list-type flow).
| declare module "express-session" { | ||
| interface SessionData { | ||
| listTypeSubscription?: ListTypeSubscriptionSession; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "listTypeSubscription" --type=ts -C3Repository: hmcts/cath-service
Length of output: 50374
🏁 Script executed:
rg -n 'declare module.*express-session' --type=ts -A5Repository: hmcts/cath-service
Length of output: 6059
Consolidate the session module augmentation to a single location.
The listTypeSubscription augmentation is duplicated across at least five files: subscription-locations-review/index.ts, subscription-list-types/index.ts, subscription-list-language/index.ts, subscription-by-location/index.ts, and subscription-confirm/index.ts. Move this declaration to a shared session types file to prevent maintenance issues and ensure consistency across the flow.
| .filter-section-heading { | ||
| font-family: "GDS Transport", arial, sans-serif; | ||
| -webkit-font-smoothing: antialiased; | ||
| -moz-osx-font-smoothing: grayscale; | ||
| font-size: 19px; | ||
| font-weight: 700; | ||
| line-height: 1.3157894737; | ||
| color: #1d70b8; | ||
| text-decoration: underline; | ||
| margin-bottom: 0; | ||
| display: block; | ||
| } | ||
|
|
||
| .filter-section-heading { | ||
| font-weight: 600; | ||
| .filter-section-toggle:hover .filter-section-heading { | ||
| color: #003078; | ||
| } | ||
|
|
||
| .filter-section-toggle:hover .filter-section-icon { | ||
| color: #003078; | ||
| } |
There was a problem hiding this comment.
Missing :focus styles on .filter-section-toggle for keyboard navigation.
The PR objectives explicitly require WCAG 2.2 AA compliance with keyboard-accessible controls. The .filter-section-toggle button has hover states for its children but no visible :focus indicator. Other interactive elements in this file (e.g., .az-navigation__link:focus at Line 110) already define focus styles.
Consider adding a focus rule consistent with the GOV.UK focus pattern used elsewhere in this file:
Proposed fix
+.filter-section-toggle:focus {
+ outline: 3px solid transparent;
+ color: `#0b0c0c`;
+ background-color: `#fd0`;
+ box-shadow: 0 -2px `#fd0`, 0 4px `#0b0c0c`;
+}
+
+.filter-section-toggle:focus .filter-section-heading {
+ color: `#0b0c0c`;
+ text-decoration: none;
+}
+
.filter-section-toggle:hover .filter-section-heading {
color: `#003078`;
}
tsconfig.json
Outdated
| "@hmcts/blob-ingestion": ["libs/api/src"], | ||
| "@hmcts/blob-ingestion/config": ["libs/api/src/config"], | ||
| "@hmcts/subscriptions": ["libs/subscriptions/src"], | ||
| "@hmcts/subscription-list-types": ["libs/subscription-list-types/src"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if any file imports from `@hmcts/subscription-list-types/config`
rg -n '@hmcts/subscription-list-types/config'Repository: hmcts/cath-service
Length of output: 204
🏁 Script executed:
# Check the package.json for `@hmcts/subscription-list-types`
find . -path "*/subscription-list-types/package.json" -type f | head -5Repository: hmcts/cath-service
Length of output: 105
🏁 Script executed:
# Check current tsconfig.json paths around the `@hmcts/subscription-list-types` entry
cat -n tsconfig.json | sed -n '25,40p'Repository: hmcts/cath-service
Length of output: 1074
🏁 Script executed:
# Check what exports are defined in subscription-list-types package.json
if [ -f "libs/subscription-list-types/package.json" ]; then
jq '.exports' libs/subscription-list-types/package.json
fiRepository: hmcts/cath-service
Length of output: 239
Add the missing /config path alias to tsconfig.json.
Code in apps/postgres/src/schema-discovery.ts imports from @hmcts/subscription-list-types/config, but the corresponding path alias is missing from tsconfig.json. Add "@hmcts/subscription-list-types/config": ["libs/subscription-list-types/src/config"] to the paths object, consistent with other packages like @hmcts/blob-ingestion/config and the package's own exports configuration.
alexbottenberg
force-pushed
the
feature/310-user-management-implementation
branch
from
c4f1564 to
b7f5fcb
Compare
Replaces Date.now() with randomUUID() to prevent test collisions when running in parallel. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update axios from 1.13.2 to 1.13.5 to address CVE with CVSS 7.5. Added resolution in package.json to force the fixed version. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolved conflicts: - e2e-tests/tests/system-admin-dashboard.spec.ts: Kept feature branch version with consolidated E2E test pattern - package.json: Merged security resolutions from both branches (qs 6.14.2, vite 7.3.1, body-parser 2.2.2, node-forge 1.3.3) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Updated User Management dashboard link from /find-users to /user-management - Updated Location Metadata dashboard link from /location-metadata to /location-metadata-search - Merged dependency resolutions: updated qs (6.14.2), tar (7.5.9), added node-forge (1.3.3) and vite (7.3.1) - Regenerated yarn.lock for consistency 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…om:hmcts/cath-service into feature/310-user-management-implementation
Update ajv from 8.17.1 to 8.18.0 to address CVE with CVSS score 5.5. Updated in: - libs/list-types/common/package.json - libs/publication/package.json 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Moved back link from page_content block to backLink block to properly override the base template's default back link, preventing duplication. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…aracters Updated validation regex to allow hyphens, underscores, @ symbols, and periods which are commonly found in User Provenance IDs. Also increased the max length from 50 to 100 characters. Changes: - Added PROVENANCE_ID_REGEX to allow alphanumeric plus -_@. - Updated validateUserProvenanceId to use new regex - Increased max length from 50 to 100 characters - Updated error message to reflect allowed characters - Added comprehensive tests for valid and invalid characters - All 35 tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
… update date format Changes: - English: Changed 'Last sign in' to 'Last verified' - English: Changed 'Never signed in' to 'Never verified' - Welsh: Changed 'Mewngofnodi diwethaf' to 'Dilyswyd diwethaf' - Welsh: Changed 'Heb fewngofnodi erioed' to 'Heb ei ddilysu erioed' - Updated date format from DD/MM/YYYY to DD/MM/YYYY HH:MM:SS for both Creation Date and Last verified - Custom formatDate function now includes hours, minutes, and seconds 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…-users The test was expecting href="/user-management" but the actual route is /find-users (based on the page directory name). Updated the test to match the actual implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…nagement-implementation
Resolve conflict in system-admin-dashboard.spec.ts: keep consolidated single-test approach with updated tile data (9 tiles including new Manage List Types tile and corrected hrefs from master). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Update seed data provenance from B2C to B2C_IDAM - Update validation, filter checkboxes, and query logic to use B2C_IDAM - Keep display label as "B2C" across filter tags, user table, and manage-user page - Fix filter tags overflowing container by adding word wrapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nagement-implementation
…github.com/hmcts/cath-service into feature/310-user-management-implementation
Changed terminology on the manage user page to use 'sign in' instead of 'verified' for better clarity: - 'Last verified' → 'Last sign in' - 'Never verified' → 'Never signed in' Updated both English and Welsh translations. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
2 participants
https://tools.hmcts.net/jira/browse/VIBE-362
Implements comprehensive user search, management, and deletion functionality for system admin users. Includes advanced filtering with removable filter tags using GOV.UK Design System components.
Features:
Technical details:
Resolves #310
Security Vulnerability Assessment
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist
Summary by CodeRabbit
New Features
Performance
UI / Styling
Tests
Documentation
Chores