VIBE-229 Reject Media Application - #161
Conversation
WalkthroughAdds a media application rejection workflow: new pages/templates (EN/CY), route handlers (reject-reasons, reject, rejected), service function to mark applications REJECTED, GOV.UK Notify support for rejection emails, updated types/tests, E2E suite replacement, and an env var for the rejection template ID. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as Admin User
participant UI as Admin UI (Rejection Pages)
participant App as Web App / Route Handlers
participant Service as Media Application Service
participant DB as Database
participant Notify as GOV.UK Notify
Admin->>UI: Open application details
UI->>App: GET /media-applications/:id/reject-reasons
App->>Service: getApplicationById(id)
Service->>DB: read application
DB-->>Service: application (PENDING)
Service-->>App: render reasons form
Admin->>UI: Select reasons & submit
UI->>App: POST /media-applications/:id/reject-reasons
App->>App: validate ≥1 reason, store in session
App-->>UI: redirect to /reject
Admin->>UI: View confirmation, submit "yes"
UI->>App: POST /media-applications/:id/reject
App->>Service: rejectApplication(id)
Service->>DB: update status -> REJECTED
DB-->>Service: updated
par Async send email
App->>Notify: sendMediaRejectionEmail(personalisation)
Notify-->>App: response / error
end
App-->>UI: redirect to /rejected
UI->>App: GET /media-applications/:id/rejected
App->>Service: getApplicationById(id)
Service->>DB: read application (REJECTED)
Service-->>App: render rejected page
App-->>UI: display confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
…t-media-application Resolved conflicts in notification service by keeping both approval and rejection email functionality from HEAD branch.
…t-media-application
…t-media-application
… journeys - Merged media-application-approval.spec.ts and media-application-rejection.spec.ts into single media-application-management.spec.ts - Reduced test count from 27 separate tests to 5 comprehensive journey tests - Combined related test cases into complete end-to-end user flows: * Approval workflow with validations, Welsh, and accessibility checks * Rejection workflow with validations, Welsh, and accessibility checks * Role-based access control verification * Comprehensive accessibility testing across all pages * Welsh language support verification across all pages - Optimized test execution by removing redundant beforeEach hooks in single-test sections - Fixed validation error message assertions to match actual implementation - All tests passing (5/5) This follows testing guidelines to minimize test count while ensuring complete coverage through comprehensive user journey tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
e2e-tests/tests/media-application-management.spec.ts (1)
67-157: Comprehensive approval journey test with good coverage.The test covers validation errors, Welsh translations, accessibility checks, and database state verification. It follows the coding guidelines by using semantic selectors like
getByRoleandgetByText.Note: Based on learnings, consider adding keyboard navigation verification (e.g., Tab through form fields, Enter to submit) within this journey test for more complete coverage.
docs/tickets/VIBE-229/plan.md (1)
28-38: Add language specifier to fenced code block.The static analysis correctly identifies that the code block should have a language specified for proper syntax highlighting.
🔎 Proposed fix
-``` +```text pages/media-applications/[id]/ ├── reject.ts # GET/POST: Rejection confirmation formlibs/admin-pages/src/media-application/service.ts (1)
24-36: LGTM!The
rejectApplicationfunction correctly implements the rejection flow, mirroring the approval logic while intentionally preserving the proof of ID file for audit purposes.Optional: Extract common validation logic
The validation logic (lines 25-33) is identical to
approveApplication(lines 7-15). Consider extracting a shared helper to reduce duplication:+async function validateApplicationForReview(id: string): Promise<MediaApplicationDetails> { + const application = await getApplicationById(id); + + if (!application) { + throw new Error("Application not found"); + } + + if (application.status !== APPLICATION_STATUS.PENDING) { + throw new Error("Application has already been reviewed"); + } + + return application; +} + export async function approveApplication(id: string): Promise<void> { - const application = await getApplicationById(id); - - if (!application) { - throw new Error("Application not found"); - } - - if (application.status !== APPLICATION_STATUS.PENDING) { - throw new Error("Application has already been reviewed"); - } + const application = await validateApplicationForReview(id); await updateApplicationStatus(id, APPLICATION_STATUS.APPROVED); if (application.proofOfIdPath) { await deleteProofOfIdFile(application.proofOfIdPath); } } export async function rejectApplication(id: string): Promise<void> { - const application = await getApplicationById(id); - - if (!application) { - throw new Error("Application not found"); - } - - if (application.status !== APPLICATION_STATUS.PENDING) { - throw new Error("Application has already been reviewed"); - } + await validateApplicationForReview(id); await updateApplicationStatus(id, APPLICATION_STATUS.REJECTED); }libs/admin-pages/src/pages/media-applications/[id]/rejected.ts (1)
20-24: Consider validating reason keys before mapping.Line 23 maps
selectedReasonskeys to localized text without validation. If the session contains an invalid key (e.g., due to tampering or code changes), the mapped value will beundefined. This could lead to display issues in the template.Recommended: Add key validation
// Get rejection reasons from session const sessionReasons = req.session?.rejectionReasons || {}; const selectedReasons = sessionReasons.selectedReasons || []; -const reasonsList = selectedReasons.map((key: string) => lang.reasons[key as keyof typeof lang.reasons]); +const validReasonKeys = Object.keys(lang.reasons); +const reasonsList = selectedReasons + .filter((key: string) => validReasonKeys.includes(key)) + .map((key: string) => lang.reasons[key as keyof typeof lang.reasons]);libs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts (1)
41-50: Mock application data contains fields not returned bygetApplicationById.The mock
mockApplicationincludesphoneNumberandcreatedAt, but according togetApplicationByIdinqueries.ts, the query returnsappliedDate(notcreatedAt) and does not includephoneNumber. While this doesn't break the tests since the mocked function returns whatever you specify, aligning mock data with the actual data shape improves test accuracy and maintainability.🔎 Suggested mock data alignment
const mockApplication = { id: "app-123", name: "John Smith", employer: "BBC", email: "john@bbc.co.uk", - phoneNumber: "07700900123", proofOfIdPath: "/uploads/proof-app-123.pdf", + proofOfIdOriginalName: "proof-app-123.pdf", status: "REJECTED" as const, - createdAt: new Date("2024-01-01") + appliedDate: new Date("2024-01-01") };Also applies to: 81-90
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.ts (1)
48-51: Manual checkbox collection could be more maintainable.The current approach manually checks each reason. Consider using a data-driven approach for easier maintenance if more reasons are added.
🔎 Alternative approach
- // Collect selected reasons - const selectedReasons: string[] = []; - if (req.body.notAccredited) selectedReasons.push("notAccredited"); - if (req.body.invalidId) selectedReasons.push("invalidId"); - if (req.body.detailsMismatch) selectedReasons.push("detailsMismatch"); + // Collect selected reasons + const REASON_KEYS = ["notAccredited", "invalidId", "detailsMismatch"] as const; + const selectedReasons = REASON_KEYS.filter((key) => req.body[key]);libs/admin-pages/src/pages/media-applications/[id]/reject.test.ts (1)
60-69: Mock application data contains fields not returned bygetApplicationById.Similar to
rejected.test.ts, the mock includesphoneNumberandcreatedAtinstead ofappliedDate. Consider aligning with the actual query return shape.Also applies to: 113-122
libs/admin-pages/src/pages/media-applications/[id]/reject.ts (2)
96-118: Consider structured logging instead of console.error with emoji.Using
console.errorwith an emoji prefix may not integrate well with structured logging systems. Consider using a proper logger that the application likely already uses.Additionally, the
BASE_URLfallback tohttps://localhost:8080could inadvertently appear in production emails if the environment variable is missing. Consider failing explicitly or using a more appropriate default.🔎 Suggested improvements
- const linkToService = process.env.BASE_URL || "https://localhost:8080"; + const linkToService = process.env.BASE_URL; + if (!linkToService) { + console.error("BASE_URL environment variable is not set"); + } await sendMediaRejectionEmail({ fullName: application.name, email: application.email, rejectReasons, - linkToService + linkToService: linkToService || "" }); } catch (error) { - console.error("❌ Failed to send rejection email:", error); + console.error("Failed to send rejection email:", error); }
101-107: Type annotation for the map callback could be more precise.The
rparameter is typed asstring[], but the mapping at line 99 produces values fromlang.reasons[key]which are indeed arrays. However, this implicit knowledge could be clearer with explicit typing in the language file interface.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
apps/web/.env.exampleapps/web/src/server.test.tsdocs/tickets/VIBE-229/implementation-summary.mddocs/tickets/VIBE-229/plan.mddocs/tickets/VIBE-229/tasks.mde2e-tests/tests/media-application-approval.spec.tse2e-tests/tests/media-application-management.spec.tslibs/admin-pages/src/media-application/service.test.tslibs/admin-pages/src/media-application/service.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/index.njklibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.njklibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/admin-pages/src/pages/media-applications/[id]/reject.njklibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-en.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.njklibs/admin-pages/src/pages/media-applications/[id]/rejected.test.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.tslibs/notification/src/govuk-notify-service.test.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/index.tslibs/system-admin-pages/src/pages/reference-data-upload/index.test.tstypes/notifications-node-client/index.d.ts
💤 Files with no reviewable changes (1)
- e2e-tests/tests/media-application-approval.spec.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/admin-pages/src/media-application/service.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/index.tslibs/system-admin-pages/src/pages/reference-data-upload/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/media-application/service.test.tslibs/notification/src/govuk-notify-service.test.tsapps/web/src/server.test.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.tstypes/notifications-node-client/index.d.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-en.tse2e-tests/tests/media-application-management.spec.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-en.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/admin-pages/src/media-application/service.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/index.tslibs/system-admin-pages/src/pages/reference-data-upload/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/media-application/service.test.tslibs/notification/src/govuk-notify-service.test.tsapps/web/src/server.test.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.tstypes/notifications-node-client/index.d.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-en.tse2e-tests/tests/media-application-management.spec.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-en.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source code using
*.test.tsor*.spec.tsnaming pattern
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/system-admin-pages/src/pages/reference-data-upload/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/media-application/service.test.tslibs/notification/src/govuk-notify-service.test.tsapps/web/src/server.test.tse2e-tests/tests/media-application-management.spec.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
libs/*/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/*/src/pages/**/*.ts: Create page controller files with GET and POST exports following the pattern:export const GET = async (req, res) => { ... }
Provide bothenandcylanguage objects in page controllers for English and Welsh support
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/system-admin-pages/src/pages/reference-data-upload/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-en.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/admin-pages/src/media-application/service.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/index.tslibs/system-admin-pages/src/pages/reference-data-upload/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/media-application/service.test.tslibs/notification/src/govuk-notify-service.test.tsapps/web/src/server.test.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.tstypes/notifications-node-client/index.d.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-en.tse2e-tests/tests/media-application-management.spec.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-en.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/admin-pages/src/media-application/service.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/index.tslibs/system-admin-pages/src/pages/reference-data-upload/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-en.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/media-application/service.test.tslibs/notification/src/govuk-notify-service.test.tsapps/web/src/server.test.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.tstypes/notifications-node-client/index.d.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-en.tse2e-tests/tests/media-application-management.spec.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-en.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
libs/*/src/pages/**/*.njk
📄 CodeRabbit inference engine (CLAUDE.md)
Nunjucks templates must extend 'layouts/base-templates.njk' and use GOV.UK Frontend component macros
Files:
libs/admin-pages/src/pages/media-applications/[id]/index.njklibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.njklibs/admin-pages/src/pages/media-applications/[id]/rejected.njklibs/admin-pages/src/pages/media-applications/[id]/reject.njk
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E tests must be located ine2e-tests/directory with*.spec.tsnaming pattern
Tag nightly-only E2E tests with@nightlyin the test title
E2E tests should use selectors in priority order: getByRole(), getByLabel(), getByText(), getByTestId()
Accessibility testing (WCAG 2.2 AA) is mandatory - include axe-core checks in E2E tests
Include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey
Do not test visual styling (fonts, colors, margins, padding) in E2E tests
Files:
e2e-tests/tests/media-application-management.spec.ts
🧠 Learnings (12)
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tse2e-tests/tests/media-application-management.spec.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.tsdocs/tickets/VIBE-229/tasks.md
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to libs/*/src/pages/**/*.ts : Provide both `en` and `cy` language objects in page controllers for English and Welsh support
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-en.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.tslibs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to libs/*/src/pages/**/*.ts : Create page controller files with GET and POST exports following the pattern: `export const GET = async (req, res) => { ... }`
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts
📚 Learning: 2025-11-20T09:59:16.776Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 106
File: libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts:84-160
Timestamp: 2025-11-20T09:59:16.776Z
Learning: In the cath-service repository, Welsh localization (lng=cy) is not required for admin screens (system-admin-pages), so locale preservation in admin screen redirects is not necessary.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject-cy.tslibs/admin-pages/src/pages/media-applications/[id]/rejected-cy.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to libs/*/src/pages/**/*.njk : Nunjucks templates must extend 'layouts/base-templates.njk' and use GOV.UK Frontend component macros
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.njklibs/admin-pages/src/pages/media-applications/[id]/rejected.njklibs/admin-pages/src/pages/media-applications/[id]/reject.njk
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to **/package.json : All packages must use `"test": "vitest run"` script in package.json
Applied to files:
libs/admin-pages/src/media-application/service.test.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Accessibility testing (WCAG 2.2 AA) is mandatory - include axe-core checks in E2E tests
Applied to files:
e2e-tests/tests/media-application-management.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Do not test visual styling (fonts, colors, margins, padding) in E2E tests
Applied to files:
e2e-tests/tests/media-application-management.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E tests should use selectors in priority order: getByRole(), getByLabel(), getByText(), getByTestId()
Applied to files:
e2e-tests/tests/media-application-management.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E tests must be located in `e2e-tests/` directory with `*.spec.ts` naming pattern
Applied to files:
e2e-tests/tests/media-application-management.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Tag nightly-only E2E tests with `nightly` in the test title
Applied to files:
e2e-tests/tests/media-application-management.spec.ts
🧬 Code graph analysis (9)
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (2)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/admin-pages/src/pages/media-applications/[id]/approve.ts (1)
POST(110-110)
libs/admin-pages/src/media-application/service.ts (2)
libs/admin-pages/src/media-application/queries.ts (2)
getApplicationById(23-39)updateApplicationStatus(41-60)libs/admin-pages/src/media-application/model.ts (1)
APPLICATION_STATUS(1-5)
libs/notification/src/govuk-notify-service.ts (4)
libs/notification/src/index.ts (1)
sendMediaRejectionEmail(1-1)types/notifications-node-client/index.d.ts (1)
NotifyClient(25-33)libs/admin-pages/src/global.d.ts (1)
NotifyClient(2-14)libs/notification/src/global.d.ts (1)
NotifyClient(2-14)
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.ts (3)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/admin-pages/src/pages/media-applications/[id]/reject.ts (2)
GET(131-131)POST(132-132)libs/auth/src/middleware/authorise.ts (1)
requireRole(12-39)
libs/admin-pages/src/pages/media-applications/[id]/reject.test.ts (6)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/admin-pages/src/pages/media-applications/[id]/reject.ts (2)
GET(131-131)POST(132-132)libs/admin-pages/src/pages/media-applications/[id]/rejected.ts (1)
GET(47-47)libs/admin-pages/src/media-application/service.ts (1)
rejectApplication(24-36)libs/notification/src/govuk-notify-service.ts (1)
sendMediaRejectionEmail(40-59)libs/notification/src/index.ts (1)
sendMediaRejectionEmail(1-1)
libs/admin-pages/src/media-application/service.test.ts (2)
libs/admin-pages/src/media-application/model.ts (1)
APPLICATION_STATUS(1-5)libs/admin-pages/src/media-application/service.ts (1)
rejectApplication(24-36)
libs/notification/src/govuk-notify-service.test.ts (1)
libs/notification/src/govuk-notify-service.ts (1)
sendMediaRejectionEmail(40-59)
libs/admin-pages/src/pages/media-applications/[id]/rejected.ts (2)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/auth/src/middleware/authorise.ts (1)
requireRole(12-39)
libs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts (3)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/admin-pages/src/pages/media-applications/[id]/reject.ts (1)
GET(131-131)libs/admin-pages/src/pages/media-applications/[id]/rejected.ts (1)
GET(47-47)
🪛 ast-grep (0.40.3)
e2e-tests/tests/media-application-management.spec.ts
[warning] 86-86: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${approvalApplicationId})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 96-96: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${approvalApplicationId}/approve)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 107-107: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${approvalApplicationId}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 114-114: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${approvalApplicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 149-149: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 177-177: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${rejectionApplicationId}/reject-reasons)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 190-190: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${rejectionApplicationId}/reject)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 201-201: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${rejectionApplicationId}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 210-210: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${rejectionApplicationId}/reject)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 213-213: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${rejectionApplicationId}/rejected)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 283-283: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${rejectionApplicationId}/reject)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 314-314: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${rejectionApplicationId}/reject)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: Test
libs/notification/src/govuk-notify-service.test.ts
[error] 1-1: AssertionError in GOV Notify Service tests: should send email with correct parameters. The mock was called with different personalisation keys: expected 'employer' and 'name' but received 'Employer' and 'full name'.
🪛 GitHub Check: Test Changed Packages
libs/notification/src/govuk-notify-service.test.ts
[failure] 55-55: src/govuk-notify-service.test.ts > GOV Notify Service > sendMediaApprovalEmail > should send email with correct parameters
AssertionError: expected "vi.fn()" to be called with arguments: [ 'test-template-id-approval', …(2) ]
Received:
1st vi.fn() call:
[
"test-template-id-approval",
"john@example.com",
{
"personalisation": {
-
"employer": "BBC", -
"name": "John Smith",
-
"Employer": "BBC", -
"full name": "John Smith", },
-
"reference": StringContaining "media-approval-",
-
},
"reference": "media-approval-1766495064088",
]
Number of calls: 1
❯ src/govuk-notify-service.test.ts:55:29
🪛 LanguageTool
docs/tickets/VIBE-229/implementation-summary.md
[style] ~17-~17: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...lications/[id]/rejected.njk- Template with success banner -libs/admin-pages/src/pages...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
docs/tickets/VIBE-229/tasks.md
[style] ~28-~28: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...pplications/[id]/rejected.njk` template with success banner - [x] Verify "Reject application...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
🪛 markdownlint-cli2 (0.18.1)
docs/tickets/VIBE-229/plan.md
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (40)
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (1)
216-259: Good addition of Welsh language validation test coverage.This test correctly validates the Welsh error message ("Rhaid dewis opsiwn") and Welsh UI strings when no radio option is selected. The test structure follows the existing patterns established for English validation.
Note: The
mockApplicationhere omitsproofOfIdOriginalName(present in other tests at line 179), resulting inproofOfIdFilename: undefinedat line 245. This appears intentional to test the undefined case, but verify this matches the expected behavior when the original filename is not set.libs/admin-pages/src/pages/media-applications/[id]/index.njk (1)
55-55: Route update aligns with the new rejection reasons workflow.The form action correctly points to the new
/reject-reasonsendpoint, introducing an intermediate step for selecting rejection reasons before the final confirmation page. This matches the broader rejection flow implementation described in the PR.apps/web/src/server.test.ts (1)
26-43: Improved Express app mocking accuracy.The updated mock correctly represents an Express app as a function with an attached
listenmethod, rather than a plain object. This is a more accurate representation of Express's actual API and improves test reliability.libs/notification/src/govuk-notify-service.test.ts (1)
108-191: New rejection email test suite is well-structured.The
sendMediaRejectionEmailtest suite correctly covers:
- Success path with proper personalisation keys (
full-name,reject-reasons,link-to-service)- API error handling
- Network error handling
The personalisation keys here align with the implementation shown in the relevant code snippet (using
"full-name","reject-reasons","link-to-service").e2e-tests/tests/media-application-management.spec.ts (4)
9-58: Solid test setup with Prisma-based fixture management.The
beforeAll/afterAllhooks properly create and clean up test data using Prisma. The.catch(() => {})pattern in cleanup is appropriate to prevent test failures if records are already deleted during the test.
86-87: ReDoS static analysis warnings are false positives here.The static analyzer flags
new RegExp()with variable input as a potential ReDoS risk. However, in this E2E test context:
- The
approvalApplicationIdandrejectionApplicationIdare UUIDs generated by Prisma inbeforeAll- These are not user-controlled inputs
- The patterns are simple path matches without complex regex features
The warnings can be safely ignored as the variable content is controlled test data.
Also applies to: 96-97, 107-108, 114-115, 149-150
238-252: Good RBAC validation for Local Admin access restrictions.The test correctly verifies that Local Admin users cannot see the media applications tile and cannot access the feature directly. Consider also verifying the response when accessing
/media-applicationsdirectly (e.g., checking for 403 or redirect to an error page).
255-287: Thorough accessibility testing across multiple pages.Good coverage of axe-core checks on all media application pages. The disabled rules (
target-size,link-name,region) should be documented or addressed in follow-up work if they represent known issues versus intentional exclusions.Are the disabled axe rules (
target-size,link-name,region) documented technical debt, or should they be addressed as part of accessibility compliance?docs/tickets/VIBE-229/plan.md (2)
74-82: Plan signature differs from actual implementation.The planned
sendMediaRejectionEmailsignature shows:{ name, email, employer }However, the actual implementation (from
govuk-notify-service.ts) uses:{ fullName, email, rejectReasons, linkToService }Consider updating this planning document to reflect the implemented API, or marking it as superseded if the implementation intentionally diverged.
190-223: Clarification questions provide good traceability.These questions are valuable for documenting assumptions and open items. Consider moving answered questions to a separate "Resolved" section as the implementation progresses, keeping the document as a living record.
apps/web/.env.example (1)
39-39: Environment variable follows established patterns.The new
GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_REJECTIONvariable is correctly placed alongside the approval template ID and follows theSCREAMING_SNAKE_CASEnaming convention per coding guidelines.libs/admin-pages/src/pages/media-applications/[id]/reject-reasons-cy.ts (1)
1-16: LGTM!The Welsh localization structure is consistent with the English version and follows the required pattern for bilingual support.
libs/notification/src/index.ts (1)
1-1: LGTM!The export correctly adds
sendMediaRejectionEmailto the public API, following ES module conventions with the.jsextension.libs/admin-pages/src/pages/media-applications/[id]/reject-reasons-en.ts (1)
1-16: LGTM!The English localization is well-structured and consistent with the Welsh translation file.
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.njk (3)
1-4: LGTM!The template correctly extends the base layout and imports the necessary GOV.UK Frontend macros.
41-60: LGTM!The checkbox items are correctly configured with appropriate values, text, names, and checked states.
40-40: The conditional syntaxif ... elseis valid in Nunjucks and works correctly inside object literals within macro parameters. This pattern is used consistently throughout the codebase (in reference-data-upload, add-region, add-jurisdiction, add-sub-jurisdiction, manual-upload, non-strategic-upload, and public-pages files). No change is needed.Likely an incorrect or invalid review comment.
libs/admin-pages/src/pages/media-applications/[id]/rejected.ts (2)
7-18: LGTM!The language selection and application retrieval logic is correct, with appropriate 404 handling.
25-45: LGTM!The template rendering logic correctly passes all required data with proper error handling.
libs/admin-pages/src/media-application/service.test.ts (1)
89-135: LGTM!The test coverage for
rejectApplicationis comprehensive, covering success, not-found, and already-reviewed scenarios. The tests correctly verify that file deletion does NOT occur during rejection, which aligns with the intended audit trail behavior.libs/admin-pages/src/pages/media-applications/[id]/rejected.njk (3)
1-3: LGTM!The template correctly extends the base layout and imports the necessary GOV.UK Frontend macros.
36-64: LGTM!The summary list construction correctly builds the application details rows with proper date formatting and conditional reasons display.
66-68: LGTM!The "What happens next" section appropriately uses the
safefilter to render the mailto link HTML and dynamically inserts the applicant's email address.libs/admin-pages/src/pages/media-applications/[id]/rejected.test.ts (1)
1-148: Well-structured test suite with good coverage.The tests cover English/Welsh rendering, 404 handling, database errors, and middleware verification. The structure follows the established patterns in the codebase.
libs/notification/src/govuk-notify-service.ts (1)
40-59: Well-structured rejection email function.The function follows the same pattern as
sendMediaApprovalEmail, with proper validation of environment variables before use and descriptive error messages.libs/admin-pages/src/pages/media-applications/[id]/rejected-cy.ts (1)
1-27: Welsh localization looks complete and well-structured.The file provides all required Welsh translations matching the English counterpart structure, including page title, table headers, reasons with HTML formatting, and error messages. This satisfies the requirement for Welsh language support in page controllers.
libs/admin-pages/src/pages/media-applications/[id]/reject.njk (1)
1-7: Template correctly extends base layout and uses GOV.UK components.The template follows the required pattern of extending
layouts/base-template.njkand properly imports GOV.UK Frontend component macros.libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.ts (1)
80-81: Exports follow the required pattern.Both GET and POST handlers are properly exported with
requireRolemiddleware protection, following the established pattern for page controllers.libs/admin-pages/src/pages/media-applications/[id]/rejected-en.ts (1)
1-27: English localization is well-structured.The file provides all required English translations with a clear structure matching the Welsh counterpart. The error messages are user-friendly and the content is appropriate for the rejection flow.
libs/admin-pages/src/pages/media-applications/[id]/reject-cy.ts (1)
1-43: Comprehensive Welsh localization for rejection confirmation.The file provides thorough Welsh translations including page content, form labels, email preview content, and error messages. This enables full Welsh language support for the rejection confirmation flow.
libs/admin-pages/src/pages/media-applications/[id]/reject.test.ts (3)
348-370: Good resilience testing for email failures.Testing that rejection still completes and redirects even when email notification fails is an important edge case. This ensures the core business logic isn't blocked by notification failures.
1-410: Comprehensive test suite with excellent coverage.The tests thoroughly cover:
- GET/POST handlers in both English and Welsh
- Validation error scenarios
- Success and failure paths for rejection
- Email notification resilience
- 404 and database error handling
- Middleware verification
This provides strong confidence in the rejection flow implementation.
338-344: No action needed—test and implementation are correctly aligned.The rejection email format with the caret character (
^) as a line continuation marker is correctly implemented inreject.ts(line 105) and properly validated by the test expectation. The implementation strips HTML tags and formats each rejection reason as${index + 1}. ${reason}\n^${explanation}, which matches the test assertion exactly.libs/admin-pages/src/pages/media-applications/[id]/reject.ts (3)
1-9: Imports follow coding guidelines.The imports correctly use workspace aliases (
@hmcts/*) and.jsextensions for relative imports, following ES module conventions.
11-51: GET handler is well-structured with proper error handling.The handler correctly implements language switching, session retrieval, and error handling. Authorization is properly delegated to the middleware. The optional chaining on
req.session?.rejectionReasonsis a good defensive pattern.
131-132: Role-based authorization correctly applied.The GET and POST exports properly use
requireRolemiddleware to restrict access to INTERNAL_ADMIN_CTSC users, following the established security pattern.docs/tickets/VIBE-229/tasks.md (1)
1-54: Comprehensive task tracking documentation.The task list provides excellent traceability for the implementation, covering service layer, notifications, page controllers, templates, and thorough testing requirements. All items are marked complete, indicating a well-planned and executed feature.
libs/admin-pages/src/pages/media-applications/[id]/reject-en.ts (2)
13-20: Verify HTML in translation strings is safely rendered.The
reasonsobject contains HTML markup including<strong>tags and anchor elements. Ensure the template renders these using safe HTML output (e.g., Nunjucks| safefilter) and that user input is never interpolated into these strings to prevent XSS.The hardcoded URL in
notAccredited(line 16) may need to be configurable if it changes across environments.
1-43: English translations structure looks good.The translation object provides comprehensive coverage for the rejection page including page metadata, table headers, rejection reasons, email preview content, radio options, and error messages. This follows the established pattern for i18n support.
docs/tickets/VIBE-229/implementation-summary.md (1)
1-152: Excellent implementation documentation.This summary provides comprehensive documentation covering:
- Files created with clear organization
- Test coverage details with specific counts
- Key implementation notes (file retention, email resilience, authorization)
- Environment variables and routes
- Acceptance criteria checklist
- Actionable next steps for deployment
The documentation clearly distinguishes the rejection flow from the approval flow (e.g., file retention behavior) and provides good guidance for deployment.
| @@ -0,0 +1,81 @@ | |||
| import { requireRole, USER_ROLES } from "@hmcts/auth"; | |||
| import "@hmcts/web-core"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all instances of @hmcts/web-core imports in TypeScript files
echo "=== Searching for @hmcts/web-core imports ==="
rg 'import.*@hmcts/web-core' --type=ts -n
echo -e "\n=== Check the reject-reasons.ts file content ==="
cat -n libs/admin-pages/src/pages/media-applications/\[id\]/reject-reasons.ts
echo -e "\n=== Check what @hmcts/web-core provides ==="
fd -e d.ts -e ts 'web-core' | head -20Repository: hmcts/cath-service
Length of output: 6748
Add explanatory comment to the side-effect import for consistency.
The import "@hmcts/web-core"; on line 2 is necessary for Express type augmentation (required for the req.session usage on lines 22 and 69). However, similar files in the codebase (non-strategic-upload/index.ts, manual-upload/index.ts, app.ts) include an explicit comment explaining this purpose. Add the comment // Import for Express type augmentation for clarity and consistency.
🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.ts around
line 2, the side-effect import `import "@hmcts/web-core";` is missing an
explanatory comment; add the comment `// Import for Express type augmentation`
immediately after that import to document that it is required for Express
typings (e.g., `req.session`) and to match the style used in other files.
| <p>{{ error }}</p> | ||
| </div> | ||
| </div> | ||
| <p><a href="/media-applications" class="govuk-link">Back to applications list</a></p> |
There was a problem hiding this comment.
Hardcoded English text in error fallback.
The "Back to applications list" text should use a localized variable to support Welsh users.
🔎 Suggested fix
- <p><a href="/media-applications" class="govuk-link">Back to applications list</a></p>
+ <p><a href="/media-applications" class="govuk-link">{{ returnLink }}</a></p>📝 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.
| <p><a href="/media-applications" class="govuk-link">Back to applications list</a></p> | |
| <p><a href="/media-applications" class="govuk-link">{{ returnLink }}</a></p> |
🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/media-applications/[id]/reject.njk around line 19
the link text "Back to applications list" is hardcoded in English; replace it
with a localized variable/translation key from the template context (e.g. use
the existing i18n/translation helper or a page-specific key) and update the
route handler that renders this template to pass the translation or i18n helper
so Welsh users see the localized string; ensure the translation key is added to
the relevant locale files.
| {{ govukErrorSummary({ | ||
| titleText: "There is a problem", | ||
| errorList: errors | ||
| }) }} |
There was a problem hiding this comment.
Hardcoded English in error summary title.
The error summary titleText: "There is a problem" should be a localized variable for Welsh support.
🔎 Suggested fix
{{ govukErrorSummary({
- titleText: "There is a problem",
+ titleText: errorSummaryTitle,
errorList: errors
}) }}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/media-applications/[id]/reject.njk around lines 22
to 25, the error summary titleText is hardcoded to English ("There is a
problem"); replace this hardcoded string with a localized variable (e.g., pass a
translated title from the template context or use the existing i18n helper) so
the titleText uses the appropriate Welsh/English translation at render time;
ensure the template receives the localized key (like t('errorSummary.title') or
context.errorSummaryTitle) and use that variable in place of the hardcoded
string.
| href: '/media-applications/' + application.id + '/proof-of-id', | ||
| text: viewLinkText, | ||
| visuallyHiddenText: 'proof of ID', | ||
| attributes: { |
There was a problem hiding this comment.
Hardcoded English in visually hidden text.
The visuallyHiddenText: 'proof of ID' is hardcoded English and won't be accessible to Welsh screen reader users. Consider using a localized variable.
🔎 Suggested fix
text: viewLinkText,
- visuallyHiddenText: 'proof of ID',
+ visuallyHiddenText: visuallyHiddenProofOfId,
attributes: {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/media-applications/[id]/reject.njk around lines 78
to 81, the visuallyHiddenText is hardcoded as 'proof of ID' which breaks
localization; replace the hardcoded string with the appropriate localized
variable (e.g., use the page/view translation helper or injected i18n key) and
pass the translation key for "proof of ID" so Welsh screen readers receive the
localized text; ensure the translation key exists in locale files and use the
same translation method used elsewhere in this template.
| {% set reasonsHtml = '' %} | ||
| {% if reasonsList and reasonsList.length > 0 %} | ||
| {% set reasonsHtml = '<ol class="govuk-list govuk-list--number">' %} | ||
| {% for reason in reasonsList %} | ||
| {% if reason[0] %} | ||
| {% set reasonsHtml = reasonsHtml + '<li>' + reason[0] %} | ||
| {% if reason[1] %} | ||
| {% set reasonsHtml = reasonsHtml + '<br>' + reason[1] %} | ||
| {% endif %} | ||
| {% set reasonsHtml = reasonsHtml + '</li>' %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% set reasonsHtml = reasonsHtml + '</ol>' %} | ||
| {% endif %} |
There was a problem hiding this comment.
Critical: Incorrect array access on string values.
The template treats each reason in reasonsList as an array (accessing reason[0] and reason[1]), but rejected.ts line 23 constructs reasonsList as an array of strings:
const reasonsList = selectedReasons.map((key: string) => lang.reasons[key as keyof typeof lang.reasons]);This produces ["Full reason text 1", "Full reason text 2", ...], not an array of arrays.
In JavaScript/Nunjucks, strings are indexable by character position, so reason[0] returns the first character (e.g., "T"), and reason[1] returns the second character (e.g., "h"). This would display <li>T<br>h</li> instead of the full rejection reason text.
🔎 Proposed fix
Option 1: Fix the template (recommended)
-{% set reasonsHtml = '<ol class="govuk-list govuk-list--number">' %}
-{% for reason in reasonsList %}
- {% if reason[0] %}
- {% set reasonsHtml = reasonsHtml + '<li>' + reason[0] %}
- {% if reason[1] %}
- {% set reasonsHtml = reasonsHtml + '<br>' + reason[1] %}
- {% endif %}
- {% set reasonsHtml = reasonsHtml + '</li>' %}
- {% endif %}
-{% endfor %}
-{% set reasonsHtml = reasonsHtml + '</ol>' %}
+{% set reasonsHtml = '<ol class="govuk-list govuk-list--number">' %}
+{% for reason in reasonsList %}
+ {% if reason %}
+ {% set reasonsHtml = reasonsHtml + '<li>' + reason + '</li>' %}
+ {% endif %}
+{% endfor %}
+{% set reasonsHtml = reasonsHtml + '</ol>' %}Option 2: Change the data structure in rejected.ts
If multi-line reasons are needed, modify rejected.ts line 23 to return tuples:
-const reasonsList = selectedReasons.map((key: string) => lang.reasons[key as keyof typeof lang.reasons]);
+const reasonsList = selectedReasons.map((key: string) => {
+ const reasonText = lang.reasons[key as keyof typeof lang.reasons];
+ // Return as array if multi-line support needed
+ return Array.isArray(reasonText) ? reasonText : [reasonText];
+});🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/media-applications/[id]/rejected.njk around lines
21 to 34, the template treats each reason as an array (using reason[0] and
reason[1]) but rejected.ts supplies an array of strings, so you should use the
full string instead of indexing characters: replace the reason[0]/reason[1]
logic with inserting the whole reason string into the <li>; if you need
preserved line breaks convert newline characters to <br> (e.g. replace newlines
with <br> before marking safe) or simply output the reason with the appropriate
Nunjucks filters (replace + safe) so the full text displays correctly;
alternatively, if you want to keep the current template, change rejected.ts to
provide arrays/tuples per reason.
| personalisation: { | ||
| "full name": data.name, | ||
| Employer: data.employer | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libs/notification/src/govuk-notify-service.ts | head -80Repository: hmcts/cath-service
Length of output: 2249
🏁 Script executed:
# Search for test files and any references to these personalisation keys
find libs/notification -type f -name "*.spec.ts" -o -name "*.test.ts" | head -20Repository: hmcts/cath-service
Length of output: 112
🏁 Script executed:
# Search for any references to the personalisation keys or templates
rg "full.name|full-name|reject-reasons|link-to-service|TEMPLATE_ID_MEDIA" libs/notification --type=ts -B2 -A2Repository: hmcts/cath-service
Length of output: 4436
🏁 Script executed:
# Check if there are any mock files or fixtures
find libs/notification -type f \( -name "*mock*" -o -name "*fixture*" -o -name "*stub*" \) | head -20Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
cat -n libs/notification/src/govuk-notify-service.test.ts | head -150Repository: hmcts/cath-service
Length of output: 5823
Fix approval email personalisation keys to match template expectations.
The approval email is sending "full name" and Employer keys, but the test expects name and employer. This mismatch will cause the email to fail at runtime. Update lines 32-35 to send the correct personalisation keys that the template expects.
🤖 Prompt for AI Agents
In libs/notification/src/govuk-notify-service.ts around lines 32 to 35, the
personalisation object uses keys "full name" and Employer which don't match the
template/tests; change the keys to name and employer so the personalisation
payload uses { name: data.name, employer: data.employer } when building the
approval email.
| }) | ||
| ); | ||
| }); | ||
| }, 10000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file around line 52 to understand the full context
cat -n libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts | sed -n '40,70p'Repository: hmcts/cath-service
Length of output: 1378
🏁 Script executed:
# Check if this file exists and get its full content to understand the test structure
wc -l libs/system-admin-pages/src/pages/reference-data-upload/index.test.tsRepository: hmcts/cath-service
Length of output: 135
🏁 Script executed:
# Search for other tests with extended timeouts in the codebase
rg -n 'it\([^,]+,[^,]+,\s*\d+\s*\)' --type ts libs --max-count 20Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Also check for Vitest timeout syntax (using vi.setConfig or test timeout)
rg -n 'timeout|setConfig' --type ts -g '*.test.ts' -g '*.spec.ts' libs | head -30Repository: hmcts/cath-service
Length of output: 682
🏁 Script executed:
# Check if other page controllers use similar dynamic import patterns in tests
rg -n 'await import' --type ts -g '*.test.ts' -g '*.spec.ts' libs | head -20Repository: hmcts/cath-service
Length of output: 2251
🏁 Script executed:
# Look at the actual page controller being tested to understand why dynamic import is needed
cat -n libs/system-admin-pages/src/pages/reference-data-upload/index.ts | head -30Repository: hmcts/cath-service
Length of output: 1217
🏁 Script executed:
# Check if there are any related changes to the imports or dependencies in index.ts
git diff HEAD -- libs/system-admin-pages/src/pages/reference-data-upload/index.ts 2>/dev/null || echo "Git diff not available, checking file size"Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check how many tests total exist in this file and their pattern
grep -c "it(" libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts
grep "it(" libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts | wc -lRepository: hmcts/cath-service
Length of output: 65
🏁 Script executed:
# Let's see all the tests in this file to confirm none have other timeout overrides
grep -n "it(" libs/system-admin-pages/src/pages/reference-data-upload/index.test.tsRepository: hmcts/cath-service
Length of output: 656
Investigate the root cause of the test timeout requirement.
This test is the only one in the file using an explicit timeout override. All seven other tests in the same file use the identical await import("./index.js") pattern without extended timeouts, and no other tests across the codebase employ explicit timeout configurations. This suggests the timeout is masking an underlying performance issue with module loading rather than a legitimate test duration requirement.
If this test is timing out intermittently at the default limit, the root cause—likely slow imports or module initialization—should be addressed rather than working around it with a timeout increase.
| export interface NotificationEmailOptions { | ||
| personalisation?: Record<string, string | number | boolean>; | ||
| reference?: string; | ||
| emailReplyToId?: string; | ||
| oneClickUnsubscribeURL?: string; | ||
| } | ||
|
|
||
| export interface NotificationResponse { | ||
| id: string; | ||
| reference?: string; | ||
| uri: string; | ||
| template: { | ||
| id: string; | ||
| version: number; | ||
| uri: string; | ||
| }; | ||
| content: { | ||
| subject: string; | ||
| body: string; | ||
| from_email: string; | ||
| }; | ||
| } | ||
|
|
||
| export class NotifyClient { | ||
| constructor(apiKey: string); | ||
| constructor(apiKey: string, baseUrl?: string); | ||
|
|
||
| sendEmail( | ||
| templateId: string, | ||
| emailAddress: string, | ||
| personalisation?: Record<string, string>, | ||
| reference?: string, | ||
| emailReplyToId?: string | ||
| ): Promise<{ | ||
| id: string; | ||
| reference?: string; | ||
| uri: string; | ||
| template: { | ||
| id: string; | ||
| version: number; | ||
| uri: string; | ||
| }; | ||
| content: { | ||
| subject: string; | ||
| body: string; | ||
| from_email: string; | ||
| }; | ||
| }>; | ||
| options?: NotificationEmailOptions | ||
| ): Promise<NotificationResponse>; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
notifications-node-client npm v8.2.1 sendEmail method signature API
💡 Result:
For notifications-node-client v8.2.1 the sendEmail method has two common forms shown in the package docs/usage:
- sendEmail(templateId, emailAddress, options) — where options is an object that may contain personalisation (object), reference (string) and emailReplyToId (string). [1][2]
Example:
notifyClient.sendEmail(
'template-id',
'user@example.com',
{ personalisation: { name: 'Jane' }, reference: 'ref-123', emailReplyToId: '465' }
).then(...).catch(...);. [1][2]
Sources: npm package README and package analysis. [1] [2]
Remove the undocumented oneClickUnsubscribeURL property or verify it against the actual package version.
The type definitions for NotificationEmailOptions and NotificationResponse follow the correct naming conventions (PascalCase for interfaces, camelCase for properties). However, the oneClickUnsubscribeURL property is not documented in the official notifications-node-client v8.2.1 API—only personalisation, reference, and emailReplyToId are documented. Either remove this property, verify it exists in a later version, or add a comment explaining its purpose.
🤖 Prompt for AI Agents
In types/notifications-node-client/index.d.ts around lines 2 to 32, the
NotificationEmailOptions interface contains an undocumented property
oneClickUnsubscribeURL; remove this property from the interface unless you can
verify it exists in the exact notifications-node-client version we target
(v8.2.1) — if it does exist in a later, intentionally targeted version, update
the package version in package.json and add a brief comment above the property
explaining its purpose and linking to the authoritative docs/change log.
🎭 Playwright E2E Test Results201 tests 201 ✅ 20m 28s ⏱️ Results for commit 8b8eda9. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/notification/src/govuk-notify-service.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/notification/src/govuk-notify-service.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/notification/src/govuk-notify-service.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source code using
*.test.tsor*.spec.tsnaming pattern
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/notification/src/govuk-notify-service.test.ts
libs/*/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/*/src/pages/**/*.ts: Create page controller files with GET and POST exports following the pattern:export const GET = async (req, res) => { ... }
Provide bothenandcylanguage objects in page controllers for English and Welsh support
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/notification/src/govuk-notify-service.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/notification/src/govuk-notify-service.test.ts
🧠 Learnings (4)
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to libs/*/src/pages/**/*.ts : Provide both `en` and `cy` language objects in page controllers for English and Welsh support
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to libs/*/src/pages/**/*.ts : Create page controller files with GET and POST exports following the pattern: `export const GET = async (req, res) => { ... }`
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
📚 Learning: 2025-11-27T14:18:22.932Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 137
File: e2e-tests/tests/create-media-account.spec.ts:51-64
Timestamp: 2025-11-27T14:18:22.932Z
Learning: For the create-media-account form in libs/public-pages, the English email validation error message (errorEmailInvalid) should be: "There is a problem - Enter a valid email address, e.g. nameexample.com" to match the Welsh translation and clearly indicate the format requirement rather than suggesting the field is empty.
Applied to files:
libs/notification/src/govuk-notify-service.test.ts
🧬 Code graph analysis (3)
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.ts (2)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/admin-pages/src/pages/media-applications/[id]/reject.ts (2)
GET(141-141)POST(142-142)
libs/admin-pages/src/pages/media-applications/[id]/reject.ts (4)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/admin-pages/src/media-application/service.ts (1)
rejectApplication(24-36)libs/notification/src/govuk-notify-service.ts (1)
sendMediaRejectionEmail(40-59)libs/notification/src/index.ts (1)
sendMediaRejectionEmail(1-1)
libs/notification/src/govuk-notify-service.test.ts (1)
libs/notification/src/govuk-notify-service.ts (1)
sendMediaRejectionEmail(40-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (3)
libs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.ts (1)
1-299: Excellent test coverage for the reject-reasons page.The test suite comprehensively covers:
- GET/POST handlers in both English and Welsh
- Session state management for rejection reasons
- Error scenarios (404, database failures)
- Validation edge cases (no selection, single/multiple selections)
- Language preservation in redirects
The tests are well-structured and follow established patterns from the codebase.
libs/notification/src/govuk-notify-service.test.ts (1)
1-270: Comprehensive test coverage for rejection email flow.The new
sendMediaRejectionEmailtest suite provides thorough coverage including:
- Environment variable validation (API key and rejection template ID)
- Successful email sending with correct personalisation keys (
"full-name","reject-reasons","link-to-service")- API error handling with appropriate error messages
- Network error handling
The tests mirror the existing approval email test structure, maintaining consistency across the notification service.
libs/admin-pages/src/pages/media-applications/[id]/reject.ts (1)
111-117: No action required — rejection reason language files correctly provide array structures.The
reject-en.tsandreject-cy.tslanguage files both definereasonswith arrays matching the expected structure. Each reason (e.g.,notAccredited,invalidId,detailsMismatch) is a 2-element array containing[reason, explanation], which aligns with the destructuring pattern at lines 113-114. Both English and Welsh language objects are present as required.
| return `${index + 1}. ${reason}\n^${explanation}`; | ||
| }) | ||
| .join("\n"); | ||
| const linkToService = process.env.BASE_URL || "https://localhost:8080"; |
There was a problem hiding this comment.
BASE_URL fallback to localhost will break email links in production.
If the BASE_URL environment variable is not configured in production, rejection emails will contain links to https://localhost:8080, rendering them unusable for applicants.
The fallback should either throw an error during startup if BASE_URL is missing, or use a production-appropriate default.
🔎 Proposed fix
- const linkToService = process.env.BASE_URL || "https://localhost:8080";
+ if (!process.env.BASE_URL) {
+ throw new Error("BASE_URL environment variable not configured");
+ }
+ const linkToService = process.env.BASE_URL;Alternatively, validate BASE_URL at application startup to fail fast rather than at runtime during rejection.
📝 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.
| const linkToService = process.env.BASE_URL || "https://localhost:8080"; | |
| if (!process.env.BASE_URL) { | |
| throw new Error("BASE_URL environment variable not configured"); | |
| } | |
| const linkToService = process.env.BASE_URL; |
🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/media-applications/[id]/reject.ts around line 118,
the current fallback to "https://localhost:8080" for process.env.BASE_URL will
produce invalid links in production; remove the localhost fallback and instead
require BASE_URL in production (throw a clear error if NODE_ENV === "production"
and BASE_URL is missing) or validate BASE_URL at application startup and fail
fast; update the code to read BASE_URL without a non-production fallback and add
an initialization-time check that logs/throws a descriptive error when BASE_URL
is unset in production so emails never contain localhost links.
| linkToService | ||
| }); | ||
| } catch (error) { | ||
| console.error("❌ Failed to send rejection email:", error); |
There was a problem hiding this comment.
Avoid logging sensitive data in error messages.
The error object logged here may contain sensitive information from the application object (name, email) and rejection reasons. According to coding guidelines, sensitive data should not be included in logs.
🔎 Proposed fix
- console.error("❌ Failed to send rejection email:", error);
+ console.error("❌ Failed to send rejection email:", {
+ message: error instanceof Error ? error.message : "Unknown error",
+ applicationId: id
+ });This logs only the error message and application ID, omitting PII like names and email addresses.
📝 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.
| console.error("❌ Failed to send rejection email:", error); | |
| console.error("❌ Failed to send rejection email:", { | |
| message: error instanceof Error ? error.message : "Unknown error", | |
| applicationId: id | |
| }); |
🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/media-applications/[id]/reject.ts around line 127,
the code logs the entire error object which may contain sensitive PII from the
application (name, email, rejection reasons); change the logging to avoid
sensitive data by logging only non-PII context (e.g., a short message,
application ID) and the error message or a safe error code, and remove or redact
any application fields from the logged output so only minimal diagnostic info is
recorded.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libs/admin-pages/src/pages/media-applications/[id]/reject.ts (2)
133-133: BASE_URL fallback to localhost will break email links in production.This issue was already flagged in a previous review. The fallback to
https://localhost:8080will produce invalid links in production rejection emails if theBASE_URLenvironment variable is not configured.
142-142: Avoid logging sensitive data in error messages.This issue was already flagged in a previous review. The error object may contain sensitive PII from the application (name, email) and rejection reasons. According to coding guidelines, sensitive data should not be included in logs.
🧹 Nitpick comments (2)
libs/admin-pages/src/pages/media-applications/[id]/reject.ts (2)
13-34: Prefer a well-known library for HTML stripping.The manual
stripHtmlTagsimplementation may miss edge cases like malformed HTML, HTML entities, or nested script tags. Consider using a mature library such asstriptagsorstrip-htmlinstead.🔎 Example using striptags library
First, install the library:
npm install striptagsThen replace the function:
+import striptags from 'striptags'; + -const MAX_REASON_LENGTH = 10000; - -function stripHtmlTags(input: string): string { - if (input.length > MAX_REASON_LENGTH) { - throw new Error("Input exceeds maximum allowed length"); - } - - let result = ""; - let insideTag = false; - - for (let i = 0; i < input.length; i++) { - const char = input[i]; - - if (char === "<") { - insideTag = true; - } else if (char === ">") { - insideTag = false; - } else if (!insideTag) { - result += char; - } - } - - return result; -}Then use
striptags(input)at lines 128-129 instead of callingstripHtmlTags.
122-132: Add defensive checks for session data structure.The code assumes
rejectionReasonsexists in session and that each reason inreasonsListis a two-element string array. If a user navigates directly to this page, or if the session data structure changes, the mapping on line 127 could throw a runtime error.Consider adding validation:
const sessionReasons = req.session?.rejectionReasons || {}; const selectedReasons = sessionReasons.selectedReasons || []; if (selectedReasons.length === 0) { // Redirect back to reject-reasons selection return res.redirect(`/media-applications/${id}/reject-reasons`); } const reasonsList = selectedReasons .map((key: string) => lang.reasons[key as keyof typeof lang.reasons]) .filter((r): r is [string, string] => Array.isArray(r) && r.length === 2);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
libs/*/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/*/src/pages/**/*.ts: Create page controller files with GET and POST exports following the pattern:export const GET = async (req, res) => { ... }
Provide bothenandcylanguage objects in page controllers for English and Welsh support
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
🧠 Learnings (2)
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to libs/*/src/pages/**/*.ts : Create page controller files with GET and POST exports following the pattern: `export const GET = async (req, res) => { ... }`
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to **/*.{ts,tsx} : Do not include sensitive data in logs
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/reject.ts
🔇 Additional comments (2)
libs/admin-pages/src/pages/media-applications/[id]/reject.ts (2)
156-157: LGTM!The exports follow the required pattern for page controllers with proper role-based access control using
INTERNAL_ADMIN_CTSC.
38-41: Add input validation for theidparameter before querying the database.The
idparameter fromreq.paramsis used directly without validation. According to coding guidelines, input validation must be performed on all endpoints. Add validation to check thatidis not empty or null before passing it togetApplicationById(). This applies to both GET and POST handlers (lines 38 and 80).
3 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-229
Change description
Reject Media Application
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.