feat(301): third-party user management - #458
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a complete third-party user management system for System Admin users, including database tables, a service layer, a multi-step web UI for creating/managing/deleting third-party users with paginated subscription configuration (A/B tested via LaunchDarkly), comprehensive E2E tests, and supporting infrastructure updates. ChangesThird-Party User Database Schema and Service Layer
Feature Flag Integration for Subscription UI
User Listing and Creation Flow
Subscription Management Flow
User Management and Deletion Flow
System Integration, Testing, and Infrastructure
Sequence Diagram(s)sequenceDiagram
actor Admin as System Admin
participant UI as Create/Manage<br/>Pages
participant Session as Express<br/>Session
participant Service as Third-Party<br/>User Service
participant DB as PostgreSQL
participant LD as LaunchDarkly<br/>(Subscriptions)
rect rgb(200, 150, 255, 0.5)
Note over Admin,DB: Create Third-Party User Flow
Admin->>UI: GET /third-party-users/create
UI->>Session: Load stored name
UI-->>Admin: Render create form
Admin->>UI: POST name
UI->>Service: validateName()
Service-->>UI: Valid ✓
UI->>Session: Store { name }
UI-->>Admin: Redirect to /create/summary
Admin->>UI: GET /create/summary
UI->>Session: Fetch stored name
UI-->>Admin: Render review page
Admin->>UI: POST confirm
UI->>Service: createThirdPartyUser(name)
Service->>DB: INSERT third_party_user
DB-->>Service: { id, name, createdAt }
Service-->>UI: User created
UI->>Session: Store { createdId, createdName }
UI-->>Admin: Redirect to /create/confirmation
Admin->>UI: GET /create/confirmation
UI->>Session: Load createdName
UI->>Session: Clear stored data
UI-->>Admin: Render success + name
end
rect rgb(200, 150, 255, 0.5)
Note over Admin,LD: Manage Subscriptions Flow
Admin->>UI: GET /[id]/subscriptions/manage?page=1
UI->>Service: findThirdPartyUserById(id)
Service->>DB: SELECT user + subscriptions
DB-->>Service: User + current subscriptions
Service-->>UI: User data
UI->>LD: isFeatureEnabled("subscription-ui-variant", userId)
LD-->>UI: Use radio buttons (true) or dropdown (false)
UI->>Session: Initialize { pending: {...} }
UI-->>Admin: Render form with controls
Admin->>UI: POST selected sensitivities (page 1)
UI->>Session: Update pending values for page 1
UI-->>Admin: Redirect to ?page=2 (next page)
Admin->>UI: POST final page sensitivities
UI->>Session: Update pending values for final page
UI->>Service: updateThirdPartySubscriptions(userId, pending)
Service->>DB: Transaction: DELETE old, INSERT new
DB-->>Service: Updated
Service-->>UI: Done
UI->>Session: Clear pending state
UI-->>Admin: Redirect to /subscriptions/success
end
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright E2E Test Results84 tests 52 ✅ 6m 12s ⏱️ Results for commit c4d70f6. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (13)
libs/system-admin-pages/src/feature-flags/launch-darkly.ts (1)
14-18: Please avoid silent LaunchDarkly initialisation failures.Line 16 swallows the error completely. Add a warning/error log (ideally once) so LD outages or bad SDK config are observable in production.
libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/confirmation/index.njk (1)
8-11: Prefertextoverhtmlfor panel body content.If
panelBodyis plain copy, render it withtextto reduce XSS exposure from future content changes.Suggested change
- {{ govukPanel({ - titleText: panelTitle, - html: panelBody - }) }} + {{ govukPanel({ + titleText: panelTitle, + text: panelBody + }) }}libs/system-admin-pages/src/pages/third-party-users/index.njk (1)
4-4: Unused import:govukBackLinkmacro.The macro is imported but the backLink block uses a plain anchor tag instead. Remove the unused import for consistency.
🧹 Proposed fix
{% from "govuk/components/table/macro.njk" import govukTable %} -{% from "govuk/components/back-link/macro.njk" import govukBackLink %}libs/system-admin-pages/src/pages/third-party-users/create/confirmation/index.njk (1)
3-3: Unused import:govukButtonmacro.The button macro is imported but not used on this confirmation page. Remove to keep imports clean.
🧹 Proposed fix
{% from "govuk/components/panel/macro.njk" import govukPanel %} -{% from "govuk/components/button/macro.njk" import govukButton %}libs/system-admin-pages/src/pages/third-party-users/create/index.njk (1)
5-5: Unused import:govukBackLinkmacro.Same as other templates - the macro is imported but the backLink block uses a plain anchor. Remove for consistency.
🧹 Proposed fix
{% from "govuk/components/error-summary/macro.njk" import govukErrorSummary %} -{% from "govuk/components/back-link/macro.njk" import govukBackLink %}libs/third-party-user/src/name-validation.ts (1)
2-2: ASCII regex limits names to basic Latin characters, inconsistent with potential international users.The regex currently restricts names to ASCII letters (a–z, A–Z). Whilst this works for English and Welsh, if third-party users include those with accented characters (é, ñ, ü) or non-Latin scripts, names will be rejected. The codebase supports only English and Welsh locales currently, so this is not an immediate concern, but consider using Unicode-aware patterns (
/^[\p{L}\p{N} '-]+$/u) if international character support becomes required.libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/index.njk (2)
1-5: Missing error handling withgovukErrorSummary.The template imports
govukRadiosbut doesn't use it (raw HTML radio inputs are used instead). More importantly, there's nogovukErrorSummaryimport or usage for displaying validation errors. As per coding guidelines, Nunjucks templates should include error handling withgovukErrorSummary.Consider adding error handling:
{% from "govuk/components/error-summary/macro.njk" import govukErrorSummary %}And conditionally render it when errors exist.
6-8: Consider using thegovukBackLinkmacro.The back link is implemented with a raw anchor element, but
govukBackLinkis imported and unused. Using the macro ensures consistent styling and behaviour.♻️ Suggested change
{% block backLink %} - <a href="/third-party-users/{{ userId }}{{ lngParam }}" class="govuk-back-link">{{ back }}</a> + {{ govukBackLink({ + href: "/third-party-users/" + userId + lngParam, + text: back + }) }} {% endblock %}libs/system-admin-pages/src/pages/third-party-users/[id]/index.ts (1)
29-29: Date locale is hardcoded regardless of language selection.The date is formatted with
"en-GB"locale even when Welsh (cy) is selected. Consider using the selected language for date formatting.♻️ Suggested fix
- createdAt: user.createdAt.toLocaleDateString("en-GB"), + createdAt: user.createdAt.toLocaleDateString(language === "cy" ? "cy-GB" : "en-GB"),libs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.test.ts (1)
24-51: Consider adding Welsh language test forgetHandler.The
postHandlertests include Welsh parameter handling (line 116-128), butgetHandlerlacks a test verifying Welsh content rendering whenreq.query.lng === "cy".libs/third-party-user/src/third-party-user-service.ts (1)
44-46: Consider handling non-existent user deletion gracefully.
prisma.deletethrowsPrismaClientKnownRequestErrorwith codeP2025if the record doesn't exist. Depending on requirements, you may want to handle this case explicitly.docs/tickets/301/tasks.md (1)
19-19: Incomplete task:CATH_LD_KEYenvironment variable configuration.This task remains unchecked. Ensure the environment variable is documented in
.env.examplebefore merging, or track it as a follow-up issue.Would you like me to open an issue to track adding
CATH_LD_KEYto.env.example?libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/index.test.ts (1)
142-185: Consider adding a test for pagination redirect.The
postHandlertests cover the final-page save scenario but don't verify the redirect to the next page whenisLastPageis false. This is an important code path in the source handler (lines 90-93).📝 Suggested test case
+ it("should redirect to next page when not on last page", async () => { + // Arrange + vi.mocked(findThirdPartyUserById).mockResolvedValue(mockUser as never); + req.session = { thirdPartySubscriptions: { userId: "user-1", pending: {} } } as never; + req.query = { page: "1" }; + req.body = { CIVIL_DAILY_CAUSE_LIST: "PUBLIC" }; + // Mock more list types to have multiple pages + vi.mocked(await import("@hmcts/list-types-common")).mockListTypes = Array(25).fill(mockUser.subscriptions[0]); + + // Act + await postHandler(req as Request, res as Response); + + // Assert + expect(res.redirect).toHaveBeenCalledWith("/third-party-users/user-1/subscriptions?page=2"); + });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c38d5512-3025-4a16-8ff4-05d82f7ec3f2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (67)
.claude/hooks/post-write.shapps/api/src/server.tsapps/postgres/prisma/migrations/20260318000000_add_third_party_user/migration.sqlapps/postgres/src/schema-discovery.tsapps/web/package.jsonapps/web/src/server.tsdocs/tickets/301/plan.mddocs/tickets/301/tasks.mddocs/tickets/301/ticket.mde2e-tests/tests/third-party-user-management.spec.tslibs/system-admin-pages/package.jsonlibs/system-admin-pages/src/feature-flags/launch-darkly.tslibs/system-admin-pages/src/pages/third-party-users/[id]/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/confirmation/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/confirmation/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/confirmation/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/confirmation/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/confirmation/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/index.tslibs/system-admin-pages/src/pages/third-party-users/create/confirmation/cy.tslibs/system-admin-pages/src/pages/third-party-users/create/confirmation/en.tslibs/system-admin-pages/src/pages/third-party-users/create/confirmation/index.njklibs/system-admin-pages/src/pages/third-party-users/create/confirmation/index.test.tslibs/system-admin-pages/src/pages/third-party-users/create/confirmation/index.tslibs/system-admin-pages/src/pages/third-party-users/create/cy.tslibs/system-admin-pages/src/pages/third-party-users/create/en.tslibs/system-admin-pages/src/pages/third-party-users/create/index.njklibs/system-admin-pages/src/pages/third-party-users/create/index.test.tslibs/system-admin-pages/src/pages/third-party-users/create/index.tslibs/system-admin-pages/src/pages/third-party-users/create/summary/cy.tslibs/system-admin-pages/src/pages/third-party-users/create/summary/en.tslibs/system-admin-pages/src/pages/third-party-users/create/summary/index.njklibs/system-admin-pages/src/pages/third-party-users/create/summary/index.test.tslibs/system-admin-pages/src/pages/third-party-users/create/summary/index.tslibs/system-admin-pages/src/pages/third-party-users/cy.tslibs/system-admin-pages/src/pages/third-party-users/en.tslibs/system-admin-pages/src/pages/third-party-users/index.njklibs/system-admin-pages/src/pages/third-party-users/index.test.tslibs/system-admin-pages/src/pages/third-party-users/index.tslibs/third-party-user/package.jsonlibs/third-party-user/prisma/schema.prismalibs/third-party-user/src/config.tslibs/third-party-user/src/index.tslibs/third-party-user/src/name-validation.test.tslibs/third-party-user/src/name-validation.tslibs/third-party-user/src/third-party-user-service.test.tslibs/third-party-user/src/third-party-user-service.tslibs/third-party-user/tsconfig.jsontsconfig.json
| # Filter to only TypeScript/JavaScript files (skip .njk, .json, .prisma, .sh, etc.) | ||
| TS_FILES="" | ||
| while IFS= read -r f; do | ||
| case "$f" in | ||
| *.ts|*.tsx|*.js|*.jsx) | ||
| if [ -f "$f" ]; then | ||
| TS_FILES="$TS_FILES $f" | ||
| fi | ||
| ;; | ||
| esac | ||
| done <<< "$FILES_TO_CHECK" | ||
|
|
||
| if [ -z "$TS_FILES" ]; then | ||
| log_hook "No TS/JS files to check" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Paths with spaces will break due to word-splitting.
The $TS_FILES variable is built by concatenation and later expanded unquoted (lines 63, 67). Filenames containing spaces will be incorrectly split.
🛠️ Suggested fix using an array
-# Filter to only TypeScript/JavaScript files (skip .njk, .json, .prisma, .sh, etc.)
-TS_FILES=""
-while IFS= read -r f; do
- case "$f" in
- *.ts|*.tsx|*.js|*.jsx)
- if [ -f "$f" ]; then
- TS_FILES="$TS_FILES $f"
- fi
- ;;
- esac
-done <<< "$FILES_TO_CHECK"
-
-if [ -z "$TS_FILES" ]; then
- log_hook "No TS/JS files to check"
- exit 0
-fi
+# Filter to only TypeScript/JavaScript files (skip .njk, .json, .prisma, .sh, etc.)
+TS_FILES=()
+while IFS= read -r f; do
+ case "$f" in
+ *.ts|*.tsx|*.js|*.jsx)
+ if [ -f "$f" ]; then
+ TS_FILES+=("$f")
+ fi
+ ;;
+ esac
+done <<< "$FILES_TO_CHECK"
+
+if [ ${`#TS_FILES`[@]} -eq 0 ]; then
+ log_hook "No TS/JS files to check"
+ exit 0
+fiThen update the biome invocations:
-if ! $BIOME_BIN format --write $TS_FILES 2>&1; then
+if ! "$BIOME_BIN" format --write "${TS_FILES[@]}" 2>&1; then
log_hook "Format had issues (non-blocking)"
fi
-if ! $BIOME_BIN check --write $TS_FILES 2>&1; then
+if ! "$BIOME_BIN" check --write "${TS_FILES[@]}" 2>&1; then
log_hook "Lint had issues (non-blocking)"
fi| - LaunchDarkly flag key: `third-party-subscriptions-radio-buttons` (false = dropdown, true = radio buttons) | ||
| - `CATH_LD_KEY` environment variable must be set for LaunchDarkly to function; the feature flag defaults to `false` (radio button variant) when unavailable. |
There was a problem hiding this comment.
Same inconsistency as in ticket.md regarding feature flag default.
Line 84 says false = dropdown, but line 85 says it defaults to "radio button variant". Please align with the correction in ticket.md.
| model ThirdPartyUser { | ||
| id String @id @default(cuid()) | ||
| name String @db.VarChar(255) | ||
| createdAt DateTime @default(now()) @map("created_at") | ||
| subscriptions ThirdPartySubscription[] | ||
|
|
||
| @@map("third_party_user") | ||
| } |
There was a problem hiding this comment.
Consider adding a unique constraint on name.
Without a unique constraint, the idempotent create logic in third-party-user-service.ts is vulnerable to race conditions, potentially creating duplicate users with the same name.
♻️ Suggested change
model ThirdPartyUser {
id String `@id` `@default`(cuid())
- name String `@db.VarChar`(255)
+ name String `@unique` `@db.VarChar`(255)
createdAt DateTime `@default`(now()) `@map`("created_at")
subscriptions ThirdPartySubscription[]
@@map("third_party_user")
}📝 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.
| model ThirdPartyUser { | |
| id String @id @default(cuid()) | |
| name String @db.VarChar(255) | |
| createdAt DateTime @default(now()) @map("created_at") | |
| subscriptions ThirdPartySubscription[] | |
| @@map("third_party_user") | |
| } | |
| model ThirdPartyUser { | |
| id String `@id` `@default`(cuid()) | |
| name String `@unique` `@db.VarChar`(255) | |
| createdAt DateTime `@default`(now()) `@map`("created_at") | |
| subscriptions ThirdPartySubscription[] | |
| @@map("third_party_user") | |
| } |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e-tests/utils/test-support-api.ts (1)
535-537: Remove the redundant cast in the name lookup.
find()already returnsThirdPartyUserRecord | undefined; the explicit cast is unnecessary and weakens type-safety if the return shape changes.Proposed tidy-up
export async function findTestThirdPartyUserByName(name: string): Promise<ThirdPartyUserRecord | null> { const users = await getTestThirdPartyUsers(); - return (users.find((u) => u.name === name) as ThirdPartyUserRecord) || null; + return users.find((u) => u.name === name) ?? null; }e2e-tests/tests/system-admin/third-party-user-management.spec.ts (1)
135-137: Prefer accessible locators for radio controls.Using raw CSS selectors for radios is brittle; use
getByRole/getByLabelfor stability and accessibility alignment.As per coding guidelines "E2E tests should ... use getByRole() first, getByLabel() for inputs, getByText() for text, and getByTestId() as last resort."
Also applies to: 195-196, 203-204
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 657a35b0-1393-44b0-a7dc-85558d882519
📒 Files selected for processing (2)
e2e-tests/tests/system-admin/third-party-user-management.spec.tse2e-tests/utils/test-support-api.ts
| @@ -0,0 +1,9 @@ | |||
| export const cy = { | |||
There was a problem hiding this comment.
Move all the files with in confirmation folder to "success" because it is a success page,
| @@ -0,0 +1,9 @@ | |||
| export const en = { | |||
There was a problem hiding this comment.
Move all the page within "delete" to "confirmation" because it is a confirmation screen.
| @@ -0,0 +1,7 @@ | |||
| export const en = { | |||
There was a problem hiding this comment.
same here. this is success page for subscription. move this to success folder.
| @@ -0,0 +1,22 @@ | |||
| export const en = { | |||
There was a problem hiding this comment.
This is manage subscription page where it shows all the lists, not "confirmation"
| @@ -0,0 +1,13 @@ | |||
| export const en = { | |||
There was a problem hiding this comment.
This is manage user screen page, move this to manage user folder.
| @@ -0,0 +1,12 @@ | |||
| export const en = { | |||
| pageTitle: "Manage third party users", | |||
There was a problem hiding this comment.
move all these files to "manage"
| CREATE TABLE "third_party_subscription" ( | ||
| "id" TEXT NOT NULL, | ||
| "third_party_user_id" TEXT NOT NULL, | ||
| "list_type" VARCHAR(100) NOT NULL, |
There was a problem hiding this comment.
This will be list_type id, we have list_type table, so this will be foreign key to list_type table.
| @@ -0,0 +1,24 @@ | |||
| -- CreateTable | |||
There was a problem hiding this comment.
oauth confirmation table is missing completly.
junaidiqbalmoj
left a comment
junaidiqbalmoj
left a comment
There was a problem hiding this comment.
This story completely missing Oauth configuration for the subscriber which we need to connect with third party.
junaidiqbalmoj
left a comment
junaidiqbalmoj
left a comment
There was a problem hiding this comment.
It is also missing connection with newly created KV for third party. How CaTH AI will connect with KV (on local, it can use az login, but for lower environment, it needs to use managed identity)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/index.ts (1)
8-17: 💤 Low valueRemove unnecessary
asyncmarker.The
getHandlerfunction contains noawaitoperations and performs only synchronous work. Theasynckeyword is unnecessary and misleading.♻️ Proposed fix
-export const getHandler = async (req: Request, res: Response) => { +export const getHandler = (req: Request, res: Response) => { const language: Language = req.query.lng === "cy" ? "cy" : "en"; const t = language === "cy" ? cy : en; const lngParam = language === "cy" ? "?lng=cy" : "";libs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.ts (1)
42-44: 💤 Low valueConsider extracting duplicate language logic.
The language-detection logic (lines 42-44) is identical to lines 28-30. Extracting to a helper function would reduce duplication.
♻️ Proposed refactor
+function getLanguageContext(req: Request) { + const language: Language = req.query.lng === "cy" ? "cy" : "en"; + const t = language === "cy" ? cy : en; + const lngParam = language === "cy" ? "?lng=cy" : ""; + return { language, t, lngParam }; +} + export const getHandler = async (req: Request, res: Response) => { - const language: Language = req.query.lng === "cy" ? "cy" : "en"; - const t = language === "cy" ? cy : en; - const lngParam = language === "cy" ? "?lng=cy" : ""; + const { t, lngParam } = getLanguageContext(req); const { id } = req.params;Apply similar changes to
postHandler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44ab2031-b551-40af-af93-370a0f27f63c
📒 Files selected for processing (9)
e2e-tests/tests/system-admin/third-party-user-management.spec.tslibs/postgres-prisma/src/schema-discovery.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/index.ts
✅ Files skipped from review due to trivial changes (2)
- libs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/en.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/delete/success/cy.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/system-admin-pages/src/pages/third-party-users/[id]/delete/index.test.ts
- e2e-tests/tests/system-admin/third-party-user-management.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
libs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/cy.ts (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude the user name in the Welsh delete title for parity and clarity.
Line 2 ignores
name, so the confirmation text is less specific than the English flow.Proposed change
- pageTitle: (_name: string) => "Ydych chi'n siŵr eich bod eisiau dileu defnyddiwr?", + pageTitle: (name: string) => `Ydych chi'n siŵr eich bod eisiau dileu ${name}?`,libs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.njk (1)
7-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwitch the back link to
govukBackLink.The template imports the macro but bypasses it with a manual anchor.
As per coding guidelines, "Nunjucks templates must extend
layouts/base-template.njk, use govuk macros from govuk/components, and include error handling with govukErrorSummary".libs/system-admin-pages/src/pages/third-party-users/[id]/manage/index.njk (1)
6-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
govukBackLinkfor the back navigation.Line 7 uses a hard-coded anchor despite importing the GOV.UK macro.
As per coding guidelines, "Nunjucks templates must extend
layouts/base-template.njk, use govuk macros from govuk/components, and include error handling with govukErrorSummary".
🧹 Nitpick comments (3)
libs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.ts (1)
62-70: ⚡ Quick winConsider wrapping delete operation in try/catch.
If
deleteThirdPartyUserthrows an exception (e.g., database failure), the audit metadata won't be set and the user will see a generic error page. Explicit error handling would allow you to log the failed attempt and show a more helpful error message.🛡️ Proposed error handling
+ try { await deleteThirdPartyUser(id); - + } catch (error) { + req.auditMetadata = { + shouldLog: true, + action: "DELETE_THIRD_PARTY_USER_FAILED", + entityInfo: `Name: ${user.name}, ID: ${id}, Error: ${error.message}` + }; + throw error; + } + req.auditMetadata = { shouldLog: true, action: "DELETE_THIRD_PARTY_USER", entityInfo: `Name: ${user.name}, ID: ${id}` }; res.redirect(`/third-party-users/${id}/delete/success${lngParam}`);libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/index.ts (2)
46-46: 💤 Low valueUnnecessary fallback for feature flag evaluation.
Since this route is protected by
requireRole([USER_ROLES.SYSTEM_ADMIN])(line 116),req.userwill always be defined. The?? "anonymous"fallback is defensive but unnecessary.♻️ Simplification
- const useDropdown = !(await isFeatureEnabled(LD_FLAG_RADIO_BUTTONS, req.user?.id ?? "anonymous")); + const useDropdown = !(await isFeatureEnabled(LD_FLAG_RADIO_BUTTONS, req.user.id));
103-113: ⚡ Quick winAdd error handling for subscription update operation.
If
updateThirdPartySubscriptionsfails (e.g., database error), the audit metadata on line 107 would still record a successful update, which is misleading. Wrap the update in try/catch to handle errors appropriately and log failed attempts.🛡️ Proposed error handling
+ try { await updateThirdPartySubscriptions(id, session.thirdPartySubscriptions.pending); + } catch (error) { + req.auditMetadata = { + shouldLog: true, + action: "UPDATE_THIRD_PARTY_SUBSCRIPTIONS_FAILED", + entityInfo: `User: ${user.name}, Error: ${error.message}` + }; + throw error; + } delete session.thirdPartySubscriptions; req.auditMetadata = { shouldLog: true, action: "UPDATE_THIRD_PARTY_SUBSCRIPTIONS", entityInfo: `User: ${user.name}, Before: [${beforeSubscriptions}], After: [${afterSubscriptions}]` };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78eff35f-90b9-4637-90c0-fc0b8cf37294
📒 Files selected for processing (27)
apps/postgres/prisma/migrations/20260318000000_add_third_party_user/migration.sqle2e-tests/tests/system-admin/third-party-user-management.spec.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/manage/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/manage/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/manage/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/manage/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/manage/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/index.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/cy.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/en.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/index.njklibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/index.test.tslibs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/index.tslibs/system-admin-pages/src/pages/third-party-users/index.njklibs/third-party-user/package.jsonlibs/third-party-user/prisma/schema.prismalibs/third-party-user/src/third-party-user-service.test.tslibs/third-party-user/src/third-party-user-service.ts
✅ Files skipped from review due to trivial changes (9)
- libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/en.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/index.njk
- libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/cy.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/manage/en.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/index.test.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/index.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/delete/confirmation/en.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/manage/cy.ts
- libs/system-admin-pages/src/pages/third-party-users/[id]/subscriptions/success/cy.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/system-admin-pages/src/pages/third-party-users/index.njk
- libs/third-party-user/prisma/schema.prisma
- libs/third-party-user/src/third-party-user-service.ts
- e2e-tests/tests/system-admin/third-party-user-management.spec.ts
- libs/third-party-user/src/third-party-user-service.test.ts
|
|
||
| type Language = "en" | "cy"; | ||
|
|
||
| export const getHandler = async (req: Request, res: Response) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid exporting getHandler if it is only used for tests.
Keep getHandler module-private and test behaviour via the exported GET chain (or a dedicated internal test helper) to avoid expanding the public module surface.
As per coding guidelines, "Only export functions intended for use outside the module - do not export functions solely to test them."
| {% block backLink %} | ||
| <a href="/third-party-users/{{ userId }}/manage{{ lngParam }}" class="govuk-back-link">{{ back }}</a> | ||
| {% endblock %} |
There was a problem hiding this comment.
Use the GOV.UK back-link macro instead of a hard-coded anchor.
This keeps the template consistent with component usage rules and the imported govukBackLink.
Proposed change
{% block backLink %}
- <a href="/third-party-users/{{ userId }}/manage{{ lngParam }}" class="govuk-back-link">{{ back }}</a>
+ {{ govukBackLink({
+ text: back,
+ href: "/third-party-users/" + userId + "/manage" + lngParam
+ }) }}
{% endblock %}As per coding guidelines, "Nunjucks templates must extend layouts/base-template.njk, use govuk macros from govuk/components, and include error handling with govukErrorSummary".
📝 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.
| {% block backLink %} | |
| <a href="/third-party-users/{{ userId }}/manage{{ lngParam }}" class="govuk-back-link">{{ back }}</a> | |
| {% endblock %} | |
| {% block backLink %} | |
| {{ govukBackLink({ | |
| text: back, | |
| href: "/third-party-users/" + userId + "/manage" + lngParam | |
| }) }} | |
| {% endblock %} |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Adds ticket, plan, and tasks documentation for issue #301 covering the full CRUD workflow for third-party users including LaunchDarkly A/B testing of the subscriptions UI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dotenv is not declared as a production dependency in apps/api or apps/web, causing ERR_MODULE_NOT_FOUND crash in Docker containers on startup. Production env vars are injected by Kubernetes — no dotenv needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Resolved conflicts by taking master's removal of channel/sensitivity from legacy third-party subscriptions (PR #691) and adopting the `t` variable naming convention and `{% block content %}` template pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove AuditLogAction override from mock so the real enum value "Update third party subscriptions" is used in the assertion - Fix Biome formatting: inline the subscriptions array on one line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…Key Vault env wiring - Change ThirdPartyUser and ThirdPartySubscription primary keys from CUID text to UUID - Add migration to convert existing columns to UUID type (truncates dev data) - Remove redundant duplicate third_party_secret migration files - Update all affected tests to use UUID-format mock IDs - Fix THIRD_PARTY_KEY_VAULT env var not being loaded by web dev scripts by adding --env-file-if-exists=../../.env to tsx invocations (matching API app pattern) - Add THIRD_PARTY_KEY_VAULT to Helm values (dev and pipeline) so deployed environments also have the vault name injected into process.env - Remove stale docs/tickets/VIBE-169 directory Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Merged sscs-daily-hearing-list, utiac-jr, utiac-statutory-appeal, and magistrates-standard-list modules from master into the feature branch. Regenerated Prisma client to expose ThirdPartyUser, ThirdPartySecret, and ThirdPartySubscription models added in this feature branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Includes updates to shared list-type utilities, location data, publication processing, notification service, audit log, test-support regions, and package dependency changes brought in from master. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ject.properties Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
10 STATUS + IMPL changes (closed issue + merged closing PR → verified): REQ-0078 (#301): implemented → verified (PR #458) REQ-0105 (#428): in_progress → verified (PR #749) REQ-0106 (#429): approved → verified (PR #761) REQ-0107 (#431): implemented → verified (PR #701) REQ-0108 (#434): approved → verified (PR #772) REQ-0109 (#436): implemented → verified (PR #727) REQ-0112 (#467): implemented → verified (PR #670) REQ-0124 (#563): approved → verified (PR #782) REQ-0135 (#569): in_progress → verified (PR #748) REQ-0137 (#729): approved → verified (PR #766) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6 participants
Summary
third-party-subscriptions-radio-buttonsflag)Test plan
third-party-subscriptions-radio-buttonsflag in LaunchDarkly — subscriptions page should show radio buttons instead of dropdownsyarn test— all unit tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests