VIBE-357 Add location metadata functionality - #319
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request implements location metadata management for displaying caution and no-list messages on court and tribunal pages. It adds a database schema with migration, backend CRUD services with validation, admin pages for managing location metadata across search, manage, delete confirmation, and success flows, and integrates metadata retrieval and display into the Summary of Publications page. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as System Admin
participant Search as Search Page
participant Manage as Manage Page
participant Delete as Delete Confirmation
participant Success as Success Page
participant DB as Database
Admin->>Search: Navigate to location metadata
Admin->>Search: Enter location name
Search->>DB: Lookup location
DB-->>Search: Return location match
Search->>Manage: Redirect with location ID
Manage->>DB: Fetch existing metadata
DB-->>Manage: Return metadata (if exists)
Manage->>Admin: Render form
alt Update/Create Path
Admin->>Manage: Submit metadata form
Manage->>DB: Create/Update metadata
DB-->>Manage: Confirm operation
Manage->>Success: Redirect with operation type
else Delete Path
Admin->>Manage: Click delete button
Manage->>Delete: Redirect to confirmation
Delete->>Admin: Request confirmation
Admin->>Delete: Confirm deletion
Delete->>DB: Delete metadata
DB-->>Delete: Confirm
Delete->>Success: Redirect to success
end
Success->>Admin: Confirm operation with link
Possibly Related Issues
Possibly Related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
KianKwa
changed the title
🎭 Playwright E2E Test Results235 tests 235 ✅ 21m 52s ⏱️ Results for commit 72fbac3. ♻️ This comment has been updated with latest results. |
…BE-357-location-metadata # Conflicts: # package.json
… feature/VIBE-357-location-metadata
…e/VIBE-357-location-metadata
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/system-admin-pages/src/pages/system-admin-dashboard/cy.ts (1)
27-27:⚠️ Potential issue | 🟡 MinorUpdate cy.ts to use correct Blob Explorer route.
The Welsh tile uses
/blob-explorer(line 27) but the correct route is/blob-explorer-locations, which is used inen.tsand all related pages. The current href incy.tsis broken.
🧹 Nitpick comments (11)
libs/public-pages/src/pages/summary-of-publications/en.ts (1)
8-10: Consider extractingfactLinkUrlto a shared constant.Embedding URLs in locale files means they'd need updating in both
en.tsandcy.tsif the URL changes. A shared constant would be more maintainable — though this is minor if the URL is stable.docs/tickets/VIBE-357/plan.md (1)
36-36: Minor: fenced code block missing a language specifier.The file-structure block on line 36 has no language tag. Adding
textorplaintextwould satisfy markdownlint MD040.libs/public-pages/src/pages/summary-of-publications/index.ts (1)
97-102: No error handling for the metadata fetch.If
getLocationMetadataByLocationIdthrows (e.g. DB connectivity issue), the request will result in an unhandled 500. This is consistent with the existinggetLocationByIdandprisma.artefact.findManycalls in the same handler, so not a regression — but worth noting as a resilience gap across the handler.libs/location/src/repository/location-metadata-queries.ts (1)
10-36: Minor duplication in trim-or-null logic between create and update.The
?.trim() || nullpattern is repeated for all four fields in bothcreateLocationMetadataRecordandupdateLocationMetadataRecord. A small helper (e.g.const trimOrNull = (s?: string) => s?.trim() || null) would DRY this up, but it's a minor nit given only two call sites.♻️ Optional: extract trim helper
+const trimOrNull = (value?: string): string | null => value?.trim() || null; + export async function createLocationMetadataRecord(data: CreateLocationMetadataInput) { const { locationId, cautionMessage, welshCautionMessage, noListMessage, welshNoListMessage } = data; return prisma.locationMetadata.create({ data: { locationId, - cautionMessage: cautionMessage?.trim() || null, - welshCautionMessage: welshCautionMessage?.trim() || null, - noListMessage: noListMessage?.trim() || null, - welshNoListMessage: welshNoListMessage?.trim() || null + cautionMessage: trimOrNull(cautionMessage), + welshCautionMessage: trimOrNull(welshCautionMessage), + noListMessage: trimOrNull(noListMessage), + welshNoListMessage: trimOrNull(welshNoListMessage) } }); }Same for
updateLocationMetadataRecord.libs/location/src/repository/location-metadata-service.ts (1)
44-51: Same TOCTOU pattern on delete.The existence check before delete is redundant if you handle the Prisma
RecordNotFounderror instead. However, the current pattern is consistent across create/update/delete and provides clear error messages, so it's a reasonable trade-off.libs/system-admin-pages/src/pages/location-metadata-manage/index.ts (2)
8-10: Language helpers duplicated across page controllers.
getLanguage,getContent, andgetLanguageParamare copy-pasted inlocation-metadata-manage,location-metadata-search, andlocation-metadata-delete-confirmation. Consider extracting them into the sharedlocation-metadata-session.ts(or a newlocation-metadata-helpers.ts) to reduce duplication.
69-81:renderWithErrorre-fetches metadata on every validation/catch error.The
getLocationMetadataByLocationIdcall insiderenderWithErrorexists only to derivehasExistingMetadata. Since theactionfield from the form already indicates whether metadata exists (create vs update), you could derivehasExistingMetadatafromactionand avoid the extra DB round-trip on each error render.e2e-tests/tests/location-metadata.spec.ts (2)
107-112: Checking CSS class is borderline visual styling testing.Line 112 asserts
toHaveClass(/govuk-panel--confirmation/). The guidelines state: "Do NOT test visual styling… Focus on user journeys, validations, and accessibility." The AxeBuilder check on Lines 119–122 already validates the panel's accessibility. Consider removing the class assertion.As per coding guidelines: "Do NOT test visual styling (font sizes, background colors, margins, padding) or UI design aspects in E2E tests."
164-185: Hardcoded Tab-press count is fragile.The keyboard navigation test presses Tab a fixed number of times (2 + 5) to reach elements. Any change in page structure will silently break this test. Prefer focusing elements directly or using
page.getByRole(...).focus()to verify focusability, then assertingtoBeFocused().libs/system-admin-pages/src/pages/location-metadata-delete-confirmation/index.ts (2)
48-53: Deletion proceeds for anyconfirmDeletevalue that isn't"no".After
validateRadioSelectionpasses (ensuring a value is present), the code only checksconfirmDelete === "no"before falling through to the delete operation. If the radio somehow sends an unexpected value (e.g., a tampered request), deletion would still execute. A stricter check would be safer.Proposed fix
if (confirmDelete === "no") { return res.redirect(`/location-metadata-manage${language === "cy" ? "?lng=cy" : ""}`); } + if (confirmDelete !== "yes") { + const { locationName, locationWelshName } = session.locationMetadata; + return res.render("location-metadata-delete-confirmation/index", { + ...content, + locationName: language === "cy" ? locationWelshName : locationName, + errors: [{ text: content.noRadioSelected }] + }); + } + try { await deleteLocationMetadata(session.locationMetadata.locationId);
9-25: Language handling is inlined here but extracted into helpers in sibling page controllers.
location-metadata-manageandlocation-metadata-searchusegetLanguage(),getContent(), andgetLanguageParam()helpers. This file repeats the same logic inline. Aligning the approach would improve consistency and DRY compliance (also flagged on the manage page).Also applies to: 27-64
| ```prisma | ||
| model LocationMetadata { | ||
| locationMetadataId String @id @default(cuid()) @map("location_metadata_id") | ||
| locationId Int @unique @map("location_id") | ||
| cautionMessage String? @map("caution_message") @db.Text | ||
| welshCautionMessage String? @map("welsh_caution_message") @db.Text | ||
| noListMessage String? @map("no_list_message") @db.Text | ||
| welshNoListMessage String? @map("welsh_no_list_message") @db.Text | ||
| createdAt DateTime @default(now()) @map("created_at") | ||
| updatedAt DateTime @updatedAt @map("updated_at") | ||
|
|
||
| location Location @relation(fields: [locationId], references: [locationId], onDelete: Cascade) | ||
|
|
||
| @@map("location_metadata") | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Plan references @default(cuid()) but commits indicate UUID is used instead.
The commit history includes "use uuid rather than cuid". If the actual schema now uses @default(uuid()), update the plan to match so it doesn't mislead future readers.
| - [ ] Write unit tests for handlers | ||
|
|
There was a problem hiding this comment.
Significant test coverage gaps remain.
Unit tests for search, manage, delete-confirmation, success handlers, and the summary-of-publications logic are all unchecked. The PR description checklist for tests is also unticked. Consider addressing these before merging to master — especially for the CRUD handlers and validation paths.
Also applies to: 36-37, 44-45, 52-53, 60-61, 66-76
|
|
||
| **AS A** Service | ||
| **I WANT** to display a caution and 'no list' message on the Summary of Publications page | ||
| **SO THAT** users are aware of these important information |
There was a problem hiding this comment.
Minor grammar issue.
"these important information" → "this important information"
| test.describe("Location Metadata Management", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.goto("/system-admin-dashboard"); | ||
| await loginWithSSO(page, process.env.SSO_TEST_SYSTEM_ADMIN_EMAIL!, process.env.SSO_TEST_SYSTEM_ADMIN_PASSWORD!); | ||
| await page.waitForURL("/system-admin-dashboard"); | ||
| }); | ||
|
|
||
| test.describe("Search Page", () => { | ||
| test("should navigate to location metadata search from dashboard", async ({ page }) => { | ||
| const manageMetadataLink = page.locator('a.admin-tile:has-text("Manage Location Metadata")'); | ||
| await expect(manageMetadataLink).toBeVisible(); | ||
| await manageMetadataLink.click(); | ||
|
|
||
| await expect(page).toHaveURL("/location-metadata-search"); | ||
|
|
||
| const heading = page.locator("h1.govuk-heading-l"); | ||
| await expect(heading).toBeVisible(); | ||
| await expect(heading).toContainText("Find the location metadata to manage"); | ||
|
|
||
| // Accessibility check | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
|
|
||
| test("should display search input with autocomplete", async ({ page }) => { | ||
| await page.goto("/location-metadata-search"); | ||
|
|
||
| const searchInput = page.getByRole("combobox", { name: /search by court or tribunal/i }); | ||
| await expect(searchInput).toBeVisible(); | ||
|
|
||
| const continueButton = page.getByRole("button", { name: /continue/i }); | ||
| await expect(continueButton).toBeVisible(); | ||
| }); | ||
|
|
||
| test("should show validation error when submitting empty form", async ({ page }) => { | ||
| await page.goto("/location-metadata-search"); | ||
|
|
||
| const continueButton = page.getByRole("button", { name: /continue/i }); | ||
| await continueButton.click(); | ||
|
|
||
| // Should show error summary | ||
| const errorSummary = page.locator(".govuk-error-summary"); | ||
| await expect(errorSummary).toBeVisible(); | ||
| }); | ||
|
|
||
| test("should display Welsh content when language is Welsh @nightly", async ({ page }) => { | ||
| await page.goto("/location-metadata-search?lng=cy"); | ||
|
|
||
| const heading = page.locator("h1.govuk-heading-l"); | ||
| await expect(heading).toContainText("Dod o hyd i'r metadata lleoliad i'w reoli"); | ||
|
|
||
| // Accessibility check in Welsh | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| test.describe("Manage Page", () => { | ||
| test("should redirect to search if accessed directly without session @nightly", async ({ page }) => { | ||
| await page.goto("/location-metadata-manage"); | ||
|
|
||
| // Should redirect to search page | ||
| await expect(page).toHaveURL("/location-metadata-search"); | ||
| }); | ||
|
|
||
| test("should have correct browser tab title @nightly", async ({ page }) => { | ||
| await page.goto("/location-metadata-search"); | ||
|
|
||
| await expect(page).toHaveTitle(/Search for location metadata/i); | ||
| }); | ||
| }); | ||
|
|
||
| test.describe("Delete Confirmation Page", () => { | ||
| test("should redirect to search if accessed directly without session @nightly", async ({ page }) => { | ||
| await page.goto("/location-metadata-delete-confirmation"); | ||
|
|
||
| // Should redirect to search page | ||
| await expect(page).toHaveURL("/location-metadata-search"); | ||
| }); | ||
|
|
||
| test("should have correct browser tab title when accessed via flow @nightly", async ({ page }) => { | ||
| // This test would need location metadata to exist | ||
| // For now, just verify redirect behavior | ||
| await page.goto("/location-metadata-delete-confirmation"); | ||
| await expect(page).toHaveURL("/location-metadata-search"); | ||
| }); | ||
| }); | ||
|
|
||
| test.describe("Success Page", () => { | ||
| test("should redirect to search if accessed directly without session @nightly", async ({ page }) => { | ||
| await page.goto("/location-metadata-success"); | ||
|
|
||
| // Success page should still render even without session | ||
| // as it uses default "created" operation | ||
| const panel = page.locator(".govuk-panel"); | ||
| await expect(panel).toBeVisible(); | ||
| }); | ||
|
|
||
| test("should display success panel with correct styling @nightly", async ({ page }) => { | ||
| await page.goto("/location-metadata-success"); | ||
|
|
||
| const panel = page.locator(".govuk-panel"); | ||
| await expect(panel).toBeVisible(); | ||
| await expect(panel).toHaveClass(/govuk-panel--confirmation/); | ||
|
|
||
| // Check for next steps link | ||
| const searchLink = page.locator('a[href*="location-metadata-search"]'); | ||
| await expect(searchLink).toBeVisible(); | ||
|
|
||
| // Accessibility check | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
|
|
||
| test("should display Welsh content when language is Welsh @nightly", async ({ page }) => { | ||
| await page.goto("/location-metadata-success?lng=cy"); | ||
|
|
||
| const panel = page.locator(".govuk-panel"); | ||
| await expect(panel).toBeVisible(); | ||
|
|
||
| // Check for Welsh text | ||
| const panelTitle = page.locator(".govuk-panel__title"); | ||
| await expect(panelTitle).toContainText("Metadata lleoliad wedi'i greu"); | ||
| }); | ||
| }); | ||
|
|
||
| test.describe("Full User Journey @nightly", () => { | ||
| test("should navigate through search to manage page flow", async ({ page }) => { | ||
| // Start from dashboard | ||
| await page.goto("/system-admin-dashboard"); | ||
|
|
||
| // Click on Manage Location Metadata tile | ||
| const manageMetadataLink = page.locator('a.admin-tile:has-text("Manage Location Metadata")'); | ||
| await manageMetadataLink.click(); | ||
|
|
||
| // Verify we're on search page | ||
| await expect(page).toHaveURL("/location-metadata-search"); | ||
|
|
||
| // Verify page elements | ||
| const heading = page.locator("h1.govuk-heading-l"); | ||
| await expect(heading).toBeVisible(); | ||
|
|
||
| const searchInput = page.locator("#location-search"); | ||
| await expect(searchInput).toBeVisible(); | ||
|
|
||
| // Run accessibility check on the full journey | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| test.describe("Keyboard Navigation @nightly", () => { | ||
| test("should allow keyboard navigation through search form", async ({ page }) => { | ||
| await page.goto("/location-metadata-search"); | ||
|
|
||
| // Tab to search input | ||
| await page.keyboard.press("Tab"); | ||
| await page.keyboard.press("Tab"); | ||
|
|
||
| // Verify we can reach the continue button via keyboard | ||
| const continueButton = page.getByRole("button", { name: /continue/i }); | ||
| await expect(continueButton).toBeVisible(); | ||
|
|
||
| // Tab to continue button and press Enter | ||
| for (let i = 0; i < 5; i++) { | ||
| await page.keyboard.press("Tab"); | ||
| } | ||
| await page.keyboard.press("Enter"); | ||
|
|
||
| // Should show error (empty form) | ||
| const errorSummary = page.locator(".govuk-error-summary"); | ||
| await expect(errorSummary).toBeVisible(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tests are fragmented — consolidate into fewer journey-based tests.
The coding guidelines state: "E2E tests should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests."
Currently there are 11 tests across 6 describe blocks. The Search Page alone has 4 separate tests (navigation, input display, validation error, Welsh content) that could be a single journey test. Similarly, the Success Page tests and the redirect-guard tests could fold into one or two journeys.
As per coding guidelines: "E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests."
|
|
||
| test.describe("Search Page", () => { | ||
| test("should navigate to location metadata search from dashboard", async ({ page }) => { | ||
| const manageMetadataLink = page.locator('a.admin-tile:has-text("Manage Location Metadata")'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer semantic locators over CSS selectors.
Multiple assertions use page.locator() with CSS class selectors (e.g., 'a.admin-tile:has-text(...)', "h1.govuk-heading-l", ".govuk-error-summary", ".govuk-panel", ".govuk-panel__title"). The guidelines prefer getByRole() first, then getByLabel(), getByText(), and getByTestId() as a last resort.
For example:
-const heading = page.locator("h1.govuk-heading-l");
+const heading = page.getByRole("heading", { level: 1 });
-const errorSummary = page.locator(".govuk-error-summary");
+const errorSummary = page.getByRole("alert");As per coding guidelines: "E2E tests should use getByRole() first, getByLabel() for inputs, getByText() for text, and getByTestId() as last resort."
Also applies to: 20-21, 48-48, 55-55, 103-103, 110-112, 115-115, 128-128, 132-132, 143-143, 150-150, 153-153, 183-183
| {% if cautionMessage %} | ||
| <div class="govuk-body">{{ cautionMessage | safe }}</div> | ||
| {% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, search for where cautionMessage and noListMessage are used
rg -n "cautionMessage|noListMessage" libs/public-pages/src/pages/summary-of-publications/ -A 2 -B 2Repository: hmcts/cath-service
Length of output: 11436
🏁 Script executed:
# Search for the controller/handler that renders this template
rg -n "summary-of-publications" libs/public-pages/src/ --type ts --type tsxRepository: hmcts/cath-service
Length of output: 89
🏁 Script executed:
# Look for LocationMetadata or related data structures
rg -n "LocationMetadata|cautionMessage|noListMessage" libs/public-pages/ --type ts --type tsx -A 3Repository: hmcts/cath-service
Length of output: 89
🏁 Script executed:
# Check if there's any sanitization middleware or utilities
fd -e ts -e tsx . libs/public-pages/src/ | head -20 | xargs rg -l "sanitize|escape|xss" 2>/dev/null || echo "Searching for sanitization patterns..."
rg -n "sanitize|DOMPurify|htmlspecialchars" libs/public-pages/ --type ts --type tsxRepository: hmcts/cath-service
Length of output: 145
🏁 Script executed:
# Search for LocationMetadata model/entity definition
rg -n "class LocationMetadata|interface LocationMetadata|type LocationMetadata" libs/public-pages/ --type tsRepository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Look for the service that retrieves location metadata
rg -n "getLocationMetadataByLocationId" libs/public-pages/src/ --type ts -A 10Repository: hmcts/cath-service
Length of output: 11733
🏁 Script executed:
# Search for any HTML sanitization or DOMPurify usage across the codebase
rg -n "sanitize|DOMPurify|escape" --type ts -iRepository: hmcts/cath-service
Length of output: 3043
🏁 Script executed:
# Check if there's a database entity or data model file
fd -n "*metadata*" libs/public-pages/ --type fRepository: hmcts/cath-service
Length of output: 292
🏁 Script executed:
# Search for any comments or documentation about cautionMessage or metadata HTML content
rg -n "cautionMessage|noListMessage" libs/public-pages/ -B 3 -A 1 | grep -i "html\|safe\|sanitize\|xss"Repository: hmcts/cath-service
Length of output: 320
🏁 Script executed:
# Check if there are any comments in the template about the safe filter
cat -n libs/public-pages/src/pages/summary-of-publications/index.njk | sed -n '20,60p'Repository: hmcts/cath-service
Length of output: 2021
🏁 Script executed:
# Look for any README or documentation in libs/public-pages about security or sanitization
find libs/public-pages -name "*.md" -o -name "*.txt" | xargs cat 2>/dev/null | grep -i "sanitize\|xss\|safe" || echo "No docs found"Repository: hmcts/cath-service
Length of output: 75
Confirm HTML sanitisation requirement for admin-entered metadata messages.
cautionMessage (line 26) and noListMessage (line 53) are sourced from the LocationMetadata database table (populated by admins) and rendered with the | safe filter, which bypasses Nunjucks auto-escaping. This creates a stored XSS vector if an admin account is compromised or misused.
Either remove the | safe filter to rely on auto-escaping (if HTML markup is not required), or implement explicit sanitisation before storage or rendering if HTML support is necessary.
| {{ govukRadios({ | ||
| idPrefix: "confirm-delete", | ||
| name: "confirmDelete", | ||
| fieldset: { | ||
| legend: { | ||
| text: radioLegend, | ||
| isPageHeading: false, | ||
| classes: "govuk-visually-hidden" | ||
| } | ||
| }, | ||
| items: [ | ||
| { | ||
| value: "yes", | ||
| text: radioYes | ||
| }, | ||
| { | ||
| value: "no", | ||
| text: radioNo | ||
| } | ||
| ] | ||
| }) }} |
There was a problem hiding this comment.
Missing inline error on the radios component.
The error summary links to #confirm-delete, but the govukRadios macro doesn't receive an errorMessage when validation fails. This means no red border or inline message on the radio group itself, breaking the expected GDS error pattern.
Proposed fix
{{ govukRadios({
idPrefix: "confirm-delete",
name: "confirmDelete",
+ errorMessage: errors[0] if errors else undefined,
fieldset: {| try { | ||
| if (action === "create") { | ||
| await createLocationMetadata({ locationId, ...formData }); | ||
| session.locationMetadata.operation = "created"; | ||
| } else if (action === "update") { | ||
| await updateLocationMetadata(locationId, formData); | ||
| session.locationMetadata.operation = "updated"; | ||
| } | ||
|
|
||
| res.redirect(`/location-metadata-success${getLanguageParam(language)}`); |
There was a problem hiding this comment.
Unrecognised action value silently redirects to success without performing any operation.
If action is neither "create", "update", nor "delete", the code falls through the if/else if block, performs no database operation, and still redirects to the success page on Line 96. This could confuse the user or mask a tampered/missing form field.
Proposed fix
if (action === "create") {
await createLocationMetadata({ locationId, ...formData });
session.locationMetadata.operation = "created";
} else if (action === "update") {
await updateLocationMetadata(locationId, formData);
session.locationMetadata.operation = "updated";
+ } else {
+ return renderWithError("An unexpected error occurred.");
}
res.redirect(`/location-metadata-success${getLanguageParam(language)}`);| hint: { | ||
| text: searchHint | ||
| }, | ||
| errorMessage: { text: "" } if errors else undefined, |
There was a problem hiding this comment.
Empty error message text on the input component.
When errors is truthy, errorMessage is set to { text: "" }, which renders the red error border on the input but displays no message beside it. This harms accessibility — screen reader users get no indication of what went wrong at the field level.
Pass the actual error text instead, e.g.:
Proposed fix
- errorMessage: { text: "" } if errors else undefined,
+ errorMessage: errors[0] if errors else undefined,📝 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.
| errorMessage: { text: "" } if errors else undefined, | |
| errorMessage: errors[0] if errors else undefined, |
| const location = await getLocationWithDetails(locationId); | ||
| if (!location) { | ||
| return redirectWithError(content.locationNotFound); | ||
| } |
There was a problem hiding this comment.
No error handling around getLocationWithDetails — inconsistent with sibling controllers.
Both location-metadata-manage and location-metadata-delete-confirmation wrap their service calls in try/catch and render errors. Here, a database failure would surface as an unhandled promise rejection rather than a user-friendly error.
Proposed fix
- const location = await getLocationWithDetails(locationId);
- if (!location) {
- return redirectWithError(content.locationNotFound);
- }
+ let location;
+ try {
+ location = await getLocationWithDetails(locationId);
+ } catch {
+ return redirectWithError(content.locationNotFound);
+ }
+ if (!location) {
+ return redirectWithError(content.locationNotFound);
+ }…e/VIBE-357-location-metadata # Conflicts: # e2e-tests/tests/care-standards-tribunal-upload.spec.ts
|
2 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-357
Change description
Add location metadata functionality
Checklist
Summary by CodeRabbit
New Features