feature/292-language-toggle-service-navigation#423
feature/292-language-toggle-service-navigation#423alexbottenberg wants to merge 5 commits intomasterfrom
Conversation
Move language toggle from inconsistent locations (phase banner with inline styles and landing page inset text) to GOV.UK service navigation component for consistent positioning across all public pages. Changes: - Add language toggle to service navigation navigationEnd slot - Add lang attribute for screen reader pronunciation - Add aria-label for accessibility (WCAG 2.2 AA compliant) - Remove language toggle from phase banner - Remove Welsh availability inset text from landing page - Update locale files: rename switchPhaseBanner to switch - Add switchAriaLabel translation key (EN/CY) - Remove hideLanguageToggle flags from admin controllers (18 files) - Update all corresponding unit tests (40+ files) - Add custom CSS class for reduced spacing (0.5rem) Admin pages correctly hide the toggle by not providing language toggle variables to templates. Fixes #292 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoves the language toggle from the phase banner/index inset into the service navigation, adds middleware to hide the toggle for specific admin roles, renames locale keys and adds an aria-label, removes hideLanguageToggle flags across admin controllers/tests, tweaks styles, and adds E2E tests and docs. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Server as AppServer
participant Middleware as locale-middleware
participant Template as service-navigation.njk
User->>Browser: Clicks/requests page
Browser->>Server: HTTP request
Server->>Middleware: Invoke translationMiddleware (req, res, next)
Middleware->>Middleware: determine locale, otherLocale, language data
alt Authenticated admin (role in SYSTEM_ADMIN / INTERNAL_ADMIN_*)
Middleware->>Server: set res.locals (no languageToggle)
else Non-admin or unauthenticated
Middleware->>Server: set res.locals (with languageToggle.link, otherLocale)
end
Server->>Template: render page with res.locals
Template->>Browser: HTML (service navigation includes toggle or not)
Browser->>User: Display page
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/tickets/292/tasks.md (1)
55-57: Pending E2E test coverage.The E2E test for the language toggle journey is unchecked. Consider adding this before merging to ensure the new service navigation language toggle works correctly across the user journey.
Would you like me to help draft an E2E test specification for this feature?
docs/tickets/292/ticket.md (2)
142-145: Documentation references outdated key name.The content table references
language.switchPhaseBanneras "existing, reused", but the implementation correctly renamed this tolanguage.switch. Consider updating the documentation to reflect the actual implementation.
172-180: Welsh content table also references outdated key.Same as above - the spec references
language.switchPhaseBannerbut implementation useslanguage.switch. The aria-label key name (language.switch) in the table also differs from implementation (language.switchAriaLabel).docs/tickets/292/plan.md (1)
79-99: Plan template pattern differs from implementation.The plan shows unconditional rendering of the language toggle, but the actual implementation in
service-navigation.njkcorrectly uses{% if languageToggle and language %}conditional. The implementation is safer - consider updating the plan to reflect the final approach.Suggested pattern update
{% endif %} + {% if languageToggle and language %} <li class="govuk-service-navigation__item govuk-service-navigation__navigation-end"> <a class="govuk-service-navigation__link" href="{{ languageToggle.link }}" lang="{{ otherLocale }}" aria-label="{{ language.switchAriaLabel }}"> {{ language.switch }} </a> </li> + {% endif %} {% endset %}
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
apps/web/src/assets/css/index.scssapps/web/src/pages/cy.tsapps/web/src/pages/en.tsapps/web/src/pages/index.njkapps/web/src/pages/index.njk.test.tsdocs/tickets/292/plan.mddocs/tickets/292/review.mddocs/tickets/292/tasks.mddocs/tickets/292/ticket.mdlibs/admin-pages/src/pages/admin-dashboard/index.test.tslibs/admin-pages/src/pages/admin-dashboard/index.tslibs/admin-pages/src/pages/manual-upload-success/index.test.tslibs/admin-pages/src/pages/manual-upload-success/index.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.tslibs/admin-pages/src/pages/manual-upload/index.test.tslibs/admin-pages/src/pages/manual-upload/index.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/pages/media-applications/[id]/approved.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.tslibs/admin-pages/src/pages/media-applications/[id]/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/index.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject-reasons.tslibs/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.test.tslibs/admin-pages/src/pages/media-applications/[id]/rejected.tslibs/admin-pages/src/pages/media-applications/index.test.tslibs/admin-pages/src/pages/media-applications/index.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.tslibs/admin-pages/src/pages/non-strategic-upload/index.test.tslibs/admin-pages/src/pages/non-strategic-upload/index.tslibs/admin-pages/src/pages/remove-list-confirmation/index.tslibs/admin-pages/src/pages/remove-list-search-results/index.tslibs/admin-pages/src/pages/remove-list-search/index.test.tslibs/admin-pages/src/pages/remove-list-search/index.tslibs/admin-pages/src/pages/remove-list-success/index.tslibs/web-core/src/locales/cy.test.tslibs/web-core/src/locales/cy.tslibs/web-core/src/locales/en.test.tslibs/web-core/src/locales/en.tslibs/web-core/src/views/components/phase-banner-content.njklibs/web-core/src/views/components/service-navigation.njk
💤 Files with no reviewable changes (6)
- libs/web-core/src/views/components/phase-banner-content.njk
- apps/web/src/pages/cy.ts
- libs/admin-pages/src/pages/non-strategic-upload/index.test.ts
- apps/web/src/pages/index.njk
- apps/web/src/pages/en.ts
- libs/admin-pages/src/pages/manual-upload/index.test.ts
| ### Verification | ||
| - [ ] Run unit tests: `yarn test` | ||
| - [ ] Run E2E tests: `yarn test:e2e:all` | ||
| - [ ] Run linter: `yarn lint:fix` | ||
| - [ ] Verify accessibility with axe-core |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read its content
if [ -f "docs/tickets/292/tasks.md" ]; then
echo "=== File exists, showing lines 71-75 ==="
sed -n '71,75p' docs/tickets/292/tasks.md
echo ""
echo "=== Full context around these lines ==="
sed -n '65,80p' docs/tickets/292/tasks.md
else
echo "File does not exist at docs/tickets/292/tasks.md"
fiRepository: hmcts/cath-service
Length of output: 697
🏁 Script executed:
# Check if package.json exists and search for test scripts
if [ -f "package.json" ]; then
echo "=== Test-related scripts in package.json ==="
grep -E '"(test|e2e|lint)' package.json | head -15
else
echo "package.json not found"
fiRepository: hmcts/cath-service
Length of output: 534
Correct the E2E test command in the verification checklist.
The checklist references yarn test:e2e:all, but the actual available script is yarn test:e2e. Additionally, accessibility verification should be integrated inline within E2E tests using AxeBuilder for WCAG 2.2 AA compliance, rather than as a separate step.
Suggested checklist update:
### Verification
- [ ] Run unit tests: `yarn test`
- [ ] Run E2E tests: `yarn test:e2e`
- [ ] Run linter: `yarn lint:fix`
- [ ] E2E tests include inline accessibility checks (AxeBuilder, zero WCAG 2.2 AA violations)
| }, | ||
| errors: [{ text: "We could not process your upload. Please try again.", href: "#" }], | ||
| hideLanguageToggle: true | ||
| errors: [{ text: "We could not process your upload. Please try again.", href: "#" }] |
There was a problem hiding this comment.
Localise the fallback upload error message.
Line 217 hard-codes English text, so Welsh users can still receive English copy on this error path.
Proposed fix
- errors: [{ text: "We could not process your upload. Please try again.", href: "#" }]
+ errors: [{ text: lang.uploadProcessingFailed, href: "#" }]Please add uploadProcessingFailed in both ./en.js and ./cy.js.
As per coding guidelines libs/*/src/pages/**/*.ts: Every page must support both English and Welsh by providing en and cy content objects to the renderer, and templates should test with ?lng=cy query parameter.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/auth/src/middleware/redirect-helpers.ts (1)
19-21: Hoist the admin route list to module scope and treat it as a constant.Line 19 recreates the route array on every call, and
adminRoutesdoes not follow the constant naming convention. Please move it to top-level asADMIN_ROUTES(readonly) and reuse it in Line 21.♻️ Proposed refactor
+const ADMIN_ROUTES: ReadonlyArray<string> = [ + "/admin-dashboard", + "/system-admin-dashboard", + "/manual-upload", + "/media-applications", + "/non-strategic-upload", + "/remove-list" +]; + export function redirectUnauthenticated(req: Request, res: Response): void { // Save the original URL to redirect after login req.session.returnTo = req.originalUrl; // Admin/internal pages should go directly to SSO when configured - // Check for all admin routes that require internal roles - const adminRoutes = ["/admin-dashboard", "/system-admin-dashboard", "/manual-upload", "/media-applications", "/non-strategic-upload", "/remove-list"]; - - const isAdminPage = adminRoutes.some((route) => req.originalUrl.startsWith(route)); + const isAdminPage = ADMIN_ROUTES.some((route) => req.originalUrl.startsWith(route));As per coding guidelines: “Constants should use SCREAMING_SNAKE_CASE” and “Module ordering: constants outside function scope at top”.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/auth/src/middleware/redirect-helpers.test.tslibs/auth/src/middleware/redirect-helpers.ts
alexbottenberg
force-pushed
the
feat/292-language-toggle-service-navigation
branch
from
d3170a0 to
f051ed0
Compare
🎭 Playwright E2E Test Results255 tests 255 ✅ 24m 36s ⏱️ Results for commit 2a6d476. ♻️ This comment has been updated with latest results. |
…t text Add comprehensive E2E test coverage for issue 292 language toggle in service navigation. Restore GOV.UK standard inset text on landing page to inform users about Welsh language availability, maintaining both the functional service navigation toggle and the informational landing page message as recommended by GOV.UK design patterns. Changes: - Add language-toggle-service-navigation.spec.ts with 6 @nightly tests covering public user journeys, admin users (no toggle), query param preservation, multi-page consistency, and language persistence - Update page-structure.spec.ts to reference service navigation instead of phase banner for language toggle location (5 tests updated) - Restore landing-page.spec.ts inset text test (was removed, now restored) - Add Welsh availability inset text to landing page (index.njk) - Add welshAvailableText and welshAvailableLink to en.ts and cy.ts - Add unit tests for landing page inset text content - Add middleware tests for admin role language toggle hiding Test coverage: - Public users see language toggle in service navigation and inset text - Admin users (SYSTEM_ADMIN, INTERNAL_ADMIN_CTSC, INTERNAL_ADMIN_LOCAL) do not see language toggle - Query parameters preserved when switching languages - Welsh translations work correctly - Accessibility standards maintained (WCAG 2.2 AA) - Keyboard navigation functional All 249 relevant E2E tests pass. Follows GOV.UK pattern of having both a functional toggle in navigation and an informational message on the start page. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
e2e-tests/tests/page-structure.spec.ts (1)
245-249: Redundant verification — consider removing.The
keyElementsloop (lines 239–244) already verifies the language toggle's visibility andhrefattribute. This additional check duplicates that assertion without adding new coverage.🧹 Proposed removal
for (const { selector, name } of keyElements) { const element = page.locator(selector).first(); await expect(element, `${name} should be visible`).toBeVisible(); const href = await element.getAttribute("href"); expect(href, `${name} should have href attribute`).toBeTruthy(); } - - // Verify language toggle is in service navigation - const serviceNav = page.locator(".govuk-service-navigation"); - const languageToggle = serviceNav.locator('a:has-text("Cymraeg")'); - await expect(languageToggle, "Language toggle should be in service navigation").toBeVisible(); });libs/web-core/src/middleware/i18n/locale-middleware.test.ts (1)
206-261: Consider parameterising the three admin-role tests.
it.each([...])would remove repetition and keep the intent in one place.e2e-tests/tests/language-toggle-service-navigation.spec.ts (2)
5-236: Please consolidate into fewer end-to-end journeys per spec.The suite is split into many narrowly scoped tests; this conflicts with the repo rule to keep E2E coverage as one complete journey with validations inline.
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."
12-229: Use role-based selectors instead of CSS and text locators throughout.The test file contains approximately 30
.locator()calls using CSS selectors and text patterns, with no role-based queries. Per coding guidelines, prefergetByRole()first, thengetByLabel()for inputs,getByText()for text, andgetByTestId()as last resort. Examples to refactor:
.locator(".govuk-service-navigation")→getByRole("navigation").locator('a:has-text("Cymraeg")')→getByRole("link", { name: /Cymraeg/ }).locator("h1")→getByRole("heading")
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/src/pages/index.njkapps/web/src/pages/index.njk.test.tse2e-tests/tests/language-toggle-service-navigation.spec.tse2e-tests/tests/page-structure.spec.tslibs/web-core/src/middleware/i18n/locale-middleware.test.tslibs/web-core/src/middleware/i18n/locale-middleware.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/pages/index.njk.test.ts
| <div class="govuk-inset-text"> | ||
| {{ welshAvailableText }} <a href="{{ languageToggle.link }}" class="govuk-link">{{ welshAvailableLink }}</a>. | ||
| {{ welshAvailableText }} <a href="?lng={{ otherLocale }}" class="govuk-link" lang="{{ otherLocale }}">{{ welshAvailableLink }}</a>. | ||
| </div> |
There was a problem hiding this comment.
Remove the landing-page language inset to avoid duplicate toggles.
Line 24 still renders a language-switch link in the inset. If the switch now lives in service navigation, this creates duplicate controls and breaks the “single consistent placement” goal.
Suggested change
- <div class="govuk-inset-text">
- {{ welshAvailableText }} <a href="?lng={{ otherLocale }}" class="govuk-link" lang="{{ otherLocale }}">{{ welshAvailableLink }}</a>.
- </div>📝 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.
| <div class="govuk-inset-text"> | |
| {{ welshAvailableText }} <a href="{{ languageToggle.link }}" class="govuk-link">{{ welshAvailableLink }}</a>. | |
| {{ welshAvailableText }} <a href="?lng={{ otherLocale }}" class="govuk-link" lang="{{ otherLocale }}">{{ welshAvailableLink }}</a>. | |
| </div> |
| // One of the toggles should be visible | ||
| const englishVisible = await englishToggle.isVisible(); | ||
| const welshVisible = await welshToggle.isVisible(); | ||
| expect(englishVisible || welshVisible).toBe(true); |
There was a problem hiding this comment.
Persistence assertion is too weak and can pass on broken behaviour.
This check is effectively tautological (English or Cymraeg visible), so it does not prove language persistence after navigation.
✅ Proposed assertion fix
- // One of the toggles should be visible
- const englishVisible = await englishToggle.isVisible();
- const welshVisible = await welshToggle.isVisible();
- expect(englishVisible || welshVisible).toBe(true);
+ // If Welsh persisted, the "English" switch should be visible
+ await expect(englishToggle).toBeVisible();
+ await expect(welshToggle).not.toBeVisible();Add Express.User interface augmentation in locale-middleware.ts to declare the role property. This fixes TypeScript compilation errors in the CI pipeline where the global type augmentation from @hmcts/auth was not visible to @hmcts/web-core package. The role property is used to determine if authenticated users are admins (SYSTEM_ADMIN, INTERNAL_ADMIN_CTSC, INTERNAL_ADMIN_LOCAL) to conditionally hide the language toggle in the service navigation. Error fixed: - TS2339: Property 'role' does not exist on type 'User' Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fix 3 failing E2E tests that were attempting to click language toggle
links on admin pages. Admin users (SYSTEM_ADMIN, INTERNAL_ADMIN_CTSC,
INTERNAL_ADMIN_LOCAL) no longer see language toggles in the service
navigation as per issue 292 requirements.
Tests fixed:
- care-standards-tribunal-upload.spec.ts (2 tests)
- "should display data source as Llwytho â Llaw in Welsh"
- "should display Welsh content throughout the list page"
- delete-court.spec.ts (1 test)
- "user can complete delete court journey with validation checks"
Solution:
Replace language toggle link clicks with direct URL navigation using
?lng=cy and ?lng=en query parameters. This allows testing Welsh
translations on admin pages without requiring the UI toggle element.
Changes:
- Replace page.getByRole("link", { name: "Cymraeg" }).click() with
page.goto(url + "?lng=cy") approach
- Add proper URL handling for existing query parameters
- Update all Welsh language switching in admin page tests
All tests now work correctly with the new language toggle behavior
where admin users don't have access to the toggle UI element.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove Welsh language testing from admin-only pages since admin users do not require Welsh language support. This simplifies test maintenance and aligns with the requirement that admin interfaces are English-only. Tests removed: - care-standards-tribunal-upload.spec.ts - "should display data source as Llwytho â Llaw in Welsh" (deleted) - "Welsh Language Support" test suite (deleted) - delete-court.spec.ts - Step 9: Welsh translation test on confirmation page (removed) - Step 14: Welsh on success page test (removed) - Updated Step 16 to use English "Home" link instead of "Hafan" Admin pages (SYSTEM_ADMIN, INTERNAL_ADMIN_CTSC, INTERNAL_ADMIN_LOCAL) now only test English content. Public user pages continue to test both English and Welsh via the service navigation language toggle. This reduces test complexity and makes it clear that Welsh support is only required for public-facing pages. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e-tests/tests/delete-court.spec.ts (1)
37-43:⚠️ Potential issue | 🟠 MajorArm the autocomplete response wait before typing to prevent flaky timeouts.
At Line 39,
selectAutocompleteOption()creates the response listener after callers invokefill(). Since Playwright'swaitForResponse()is edge-triggered, it can miss responses if the request fires before the listener is registered, causing intermittent timeouts.Move
waitForResponse()andfill()into the same helper, armed before the request is triggered. Update all 4 call sites (lines 149, 188, 277, 319) to pass the inputLocator.Suggested fix
-import type { Page } from "@playwright/test"; +import type { Locator, Page } from "@playwright/test"; -async function selectAutocompleteOption(page: Page, searchText: string) { - // Wait for the autocomplete API request to complete - const responsePromise = page.waitForResponse((response) => response.url().includes("/locations?q=") && response.status() === 200, { timeout: 10000 }); +async function selectAutocompleteOption(page: Page, courtInput: Locator, searchText: string) { + // Arm wait before triggering the request + const responsePromise = page.waitForResponse( + response => + response.url().includes(`/locations?q=${encodeURIComponent(searchText)}`) && + response.status() === 200, + { timeout: 10000 } + ); + await courtInput.fill(searchText); // Wait for the response to complete await responsePromise; @@ - await courtInput.fill("Delete Test Court A"); - - // Wait for autocomplete to load and select option - await selectAutocompleteOption(page, "Delete Test Court A"); + await selectAutocompleteOption(page, courtInput, "Delete Test Court A"); @@ - await courtInput2.fill("Delete Test Court B"); - - // Wait for autocomplete to load and select option - await selectAutocompleteOption(page, "Delete Test Court B"); + await selectAutocompleteOption(page, courtInput2, "Delete Test Court B"); @@ - await courtInput.fill(courtName); - - // Wait for autocomplete to load and select option - await selectAutocompleteOption(page, courtName); + await selectAutocompleteOption(page, courtInput, courtName); @@ - await courtInput.fill("Delete Test Court C"); - - // Wait for autocomplete to load and select option - await selectAutocompleteOption(page, "Delete Test Court C"); + await selectAutocompleteOption(page, courtInput, "Delete Test Court C");
🧹 Nitpick comments (3)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (3)
442-446: Consider using the URL API for more robust query parameter handling.The string-based URL construction works for typical cases but won't handle edge cases (e.g., if
lngalready exists in the URL, or URL contains a hash fragment). The URL API provides cleaner handling.♻️ Suggested refactor using URL API
// Switch to Welsh using query parameter (admin users don't have language toggle) - const currentUrl = page.url(); - const urlWithWelsh = currentUrl.includes("?") ? `${currentUrl}&lng=cy` : `${currentUrl}?lng=cy`; - await page.goto(urlWithWelsh); + const url = new URL(page.url()); + url.searchParams.set("lng", "cy"); + await page.goto(url.toString());
572-576: Same URL construction pattern—apply the same URL API refactor here.For consistency and robustness, use the same
URLAPI approach suggested above.♻️ Suggested refactor
// Switch to Welsh using query parameter (admin users don't have language toggle) - const currentUrl = page.url(); - const urlWithWelsh = currentUrl.includes("?") ? `${currentUrl}&lng=cy` : `${currentUrl}?lng=cy`; - await page.goto(urlWithWelsh); + const url = new URL(page.url()); + url.searchParams.set("lng", "cy"); + await page.goto(url.toString());
167-167: Consider replacing fixed timeout with a condition-based wait.
waitForTimeout(1000)is a flaky pattern in E2E tests. If the page needs time to stabilise, prefer waiting for a specific element or network state.♻️ Suggested approach
await page.goto("/non-strategic-upload?locationId=9001"); - await page.waitForTimeout(1000); + await page.waitForLoadState("networkidle");Note: This pattern appears in multiple places (lines 167, 216, 237, 467, 496, 500, 511, 549). A similar refactor could be applied throughout if stability permits.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/tests/care-standards-tribunal-upload.spec.tse2e-tests/tests/delete-court.spec.ts
e2e-tests/tests/delete-court.spec.ts
Outdated
| const urlWithWelsh = currentUrl.includes("?") ? `${currentUrl}&lng=cy` : `${currentUrl}?lng=cy`; | ||
| await page.goto(urlWithWelsh); |
There was a problem hiding this comment.
Use URL.searchParams.set for lng instead of string concatenation.
At Line 195, Line 202, and Line 230, appending ?lng=/&lng= can produce duplicate lng params when one already exists, which makes locale assertions brittle.
Suggested fix
+const withLanguage = (rawUrl: string, lng: "en" | "cy"): string => {
+ const url = new URL(rawUrl);
+ url.searchParams.set("lng", lng);
+ return url.toString();
+};
-const urlWithWelsh = currentUrl.includes("?") ? `${currentUrl}&lng=cy` : `${currentUrl}?lng=cy`;
+const urlWithWelsh = withLanguage(currentUrl, "cy");
await page.goto(urlWithWelsh);
-const urlWithEnglish = currentUrl.includes("?") ? `${currentUrl}&lng=en` : `${currentUrl}?lng=en`;
+const urlWithEnglish = withLanguage(currentUrl, "en");
await page.goto(urlWithEnglish);
-const successUrlWithWelsh = successUrl.includes("?") ? `${successUrl}&lng=cy` : `${successUrl}?lng=cy`;
+const successUrlWithWelsh = withLanguage(successUrl, "cy");
await page.goto(successUrlWithWelsh);Also applies to: 202-203, 230-231
|
2 participants
Move language toggle from inconsistent locations (phase banner with inline styles and landing page inset text) to GOV.UK service navigation component for consistent positioning across all public pages.
Changes:
Admin pages correctly hide the toggle by not providing language toggle variables to templates.
Fixes #292
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist
Summary by CodeRabbit