fix: update audit log back link text and fix date filter row layout #670
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| # Plan: #467 Audit Log View Formatting Fixes | ||
|
|
||
| ## 1. Technical Approach | ||
|
|
||
| Two isolated, low-risk changes: | ||
|
|
||
| 1. Update the `backToList` string values in the English and Welsh content files for the audit-log-detail page. The template already uses `backToListText` as a variable (line 5 of `index.njk`) and the controller passes it through — no template or controller changes are needed. | ||
|
|
||
| 2. Remove the CSS rule in `dashboard.scss` that forces the Year date input onto a new row, and remove the now-unused `app-date-filter-narrow` class from the `govukDateInput` `formGroup` in the audit-log-list template. The GOV.UK `govukDateInput` macro renders Day/Month/Year on a single flex row by default; the CSS rule is the sole cause of the wrapping. | ||
|
|
||
| Neither change touches routing, business logic, or database code. | ||
|
|
||
| --- | ||
|
|
||
| ## 2. Implementation Details | ||
|
|
||
| ### Fix 1 — Back link text | ||
|
|
||
| **File:** `libs/system-admin-pages/src/pages/audit-log-detail/en.ts` | ||
|
|
||
| | Line | Current value | New value | | ||
| |------|--------------|-----------| | ||
| | 44 | `backToList: "Back to audit log list"` | `backToList: "Back"` | | ||
|
|
||
| **File:** `libs/system-admin-pages/src/pages/audit-log-detail/cy.ts` | ||
|
|
||
| | Line | Current value | New value | | ||
| |------|--------------|-----------| | ||
| | 44 | `backToList: "Yn ôl i restr log archwilio"` | `backToList: "Yn ôl"` | | ||
|
|
||
| The template (`audit-log-detail/index.njk`, line 5) already renders: | ||
| ```njk | ||
| <a href="/audit-log-list" class="govuk-back-link">{{ backToListText }}</a> | ||
| ``` | ||
| No template change required. The `href` value (`/audit-log-list`) is unchanged. | ||
|
|
||
| ### Fix 2 — Date filter inputs on one row | ||
|
|
||
| **File:** `libs/system-admin-pages/src/assets/css/dashboard.scss` | ||
|
|
||
| Remove lines 43–45 in their entirety: | ||
| ```scss | ||
| .app-date-filter-narrow .govuk-date-input__item:last-child { | ||
| padding-top: 10px; | ||
| } | ||
| ``` | ||
| After removal the `.app-date-filter-narrow` class has no associated styles in this file and no purpose elsewhere. | ||
|
|
||
| **File:** `libs/system-admin-pages/src/pages/audit-log-list/index.njk` | ||
|
|
||
| Remove the `formGroup` option from the `govukDateInput` call at lines 99–101: | ||
| ```njk | ||
| formGroup: { | ||
| classes: "app-date-filter-narrow" | ||
| }, | ||
| ``` | ||
| The surrounding `govukDateInput` call (lines 96–132) is otherwise unchanged. | ||
|
|
||
| ### Fix 3 — Update test assertion | ||
|
|
||
| **File:** `libs/system-admin-pages/src/pages/audit-log-detail/index.test.ts` | ||
|
|
||
| The test at lines 69–105 ("should render audit log detail when found") asserts: | ||
| ```typescript | ||
| backToListText: expect.any(String), | ||
| ``` | ||
| This assertion continues to pass after the change because the value is still a string. No test update is required unless the team wants to assert the exact string value. The existing assertion is sufficient for the scope of this ticket. | ||
|
|
||
|
Comment on lines
+63
to
+68
Contributor
There was a problem hiding this comment. Align test guidance with the ticket acceptance checks. Line 67 says no test update is required, but the ticket/PR test plan expects validating exact back link text in English and Welsh. Please make the plan explicit and consistent to avoid conflicting implementation guidance. |
||
| --- | ||
|
|
||
| ## 3. Error Handling & Edge Cases | ||
|
|
||
| - The back link `href` is hardcoded in the template as `/audit-log-list` and is not altered by this change. | ||
| - Removing the CSS rule cannot break layout elsewhere: the selector `.app-date-filter-narrow` is used only in this one SCSS file and only applied in `audit-log-list/index.njk`. No other template in the codebase uses that class. | ||
| - Welsh content: `"Yn ôl"` is the standard GOV.UK Wales back link text, consistent with other pages in CaTH (e.g., `audit-log-list/index.njk` line 10 uses the hardcoded `Back` for its own back link). | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Acceptance Criteria Mapping | ||
|
|
||
| | AC | How it is satisfied | | ||
| |----|-------------------| | ||
| | Back link reads "Back" (English) | `en.ts` line 44: `backToList` changed to `"Back"` | | ||
| | Back link reads "Yn ôl" (Welsh) | `cy.ts` line 44: `backToList` changed to `"Yn ôl"` | | ||
| | Day/Month/Year inputs on a single row | CSS rule removed from `dashboard.scss`; `app-date-filter-narrow` class removed from template | | ||
|
|
||
| --- | ||
|
|
||
| ## 5. Open Questions | ||
|
|
||
| None. The spec is complete and unambiguous. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| # Code Review: Issue #467 | ||
|
|
||
| ## Summary | ||
|
|
||
| Four files were changed to fix two formatting inconsistencies in the Audit Log area: the back link text on the detail page and the date filter row layout on the list page. The changes are minimal, correctly scoped, and all 637 existing tests pass. Both acceptance criteria are satisfied. No new security, type safety, or performance risks are introduced. | ||
|
|
||
| One pre-existing issue is noted in the template that is outside the scope of this ticket but worth tracking. | ||
|
|
||
| --- | ||
|
|
||
| ## CRITICAL Issues | ||
|
|
||
| None. | ||
|
|
||
| --- | ||
|
|
||
| ## HIGH PRIORITY Issues | ||
|
|
||
| None introduced by this change. | ||
|
|
||
| --- | ||
|
|
||
| ## SUGGESTIONS | ||
|
|
||
| ### 1. Test assertion uses `expect.any(String)` for `backToListText` — consider asserting the exact value | ||
|
|
||
| **File:** `libs/system-admin-pages/src/pages/audit-log-detail/index.test.ts` line 100 | ||
|
|
||
| The `backToListText` assertion at line 100 is: | ||
| ```typescript | ||
| backToListText: expect.any(String), | ||
| ``` | ||
| The plan explicitly acknowledges this and decides not to assert the exact value. That is a reasonable call given the scope, but the Welsh locale test at line 155 also does not assert `backToListText` at all. A future regression that sets `backToList` back to the old verbose string would go undetected. Adding exact-value assertions for both English and Welsh would give a tighter safety net at negligible cost: | ||
|
|
||
| ```typescript | ||
| // English case (line 100) | ||
| backToListText: "Back", | ||
|
|
||
| // Welsh case — add to the expect.objectContaining call in the cy test | ||
| backToListText: "Yn ôl", | ||
| ``` | ||
|
|
||
| ### 2. The `{% block content %}` override in both audit-log templates bypasses the `{% block page_content %}` convention | ||
|
|
||
| **Files:** `libs/system-admin-pages/src/pages/audit-log-detail/index.njk` line 8, `libs/system-admin-pages/src/pages/audit-log-list/index.njk` line 13 | ||
|
|
||
| Both templates override `{% block content %}` directly rather than `{% block page_content %}`. The base template (`libs/web-core/src/views/layouts/base-template.njk`) nests `page_content` inside `content`. Overriding `content` means the `error_summary` block defined in the layout is also bypassed. This is pre-existing and outside the scope of this ticket, but it should be tracked for a follow-up refactor to align with the `page_content` convention used elsewhere (e.g., error pages, accessibility statement). | ||
|
|
||
| ### 3. The back link `href` is hardcoded in the template rather than passed from the controller | ||
|
|
||
| **File:** `libs/system-admin-pages/src/pages/audit-log-detail/index.njk` line 5 | ||
|
|
||
| ```njk | ||
| <a href="/audit-log-list" class="govuk-back-link">{{ backToListText }}</a> | ||
| ``` | ||
|
|
||
| The text is correctly driven by a content variable, but the `href` is hardcoded. If the route ever changes, the template needs updating separately. This is pre-existing and out of scope here, but worth noting. The base template's JavaScript (`bodyEnd` block) also intercepts clicks on `.govuk-back-link` elements and calls `history.back()`, which means at runtime the hardcoded `/audit-log-list` href is never actually followed — the browser history is used instead. This is consistent with the `audit-log-list` page's own back link and appears to be intentional project convention, but it is worth understanding for anyone maintaining the template. | ||
|
|
||
| ### 4. Stale dist files contain the old CSS rule | ||
|
|
||
| The `libs/system-admin-pages/dist/` and `apps/web/dist/` directories still contain the old rule (`.app-date-filter-narrow .govuk-date-input__item:last-child { padding-top: 10px; }`). These are build artefacts and not committed, so this is not a problem for production deployment provided the build runs as part of the CI/CD pipeline. Confirm the pipeline runs `yarn build` before deploying. | ||
|
|
||
| --- | ||
|
|
||
| ## Positive Feedback | ||
|
|
||
| - The change is minimal and precisely scoped to the two acceptance criteria. No unrelated files were touched. | ||
| - The CSS fix removes the rule entirely rather than trying to override it, which is the cleanest approach. | ||
| - The `app-date-filter-narrow` class was also removed from the Nunjucks template, eliminating a now-meaningless class reference. | ||
| - Content is driven through the `en.ts`/`cy.ts` content files as intended by the architecture — the template was not touched for the back link text change. | ||
| - The Welsh translation `"Yn ôl"` is correct standard GOV.UK Wales back link text, consistent with the project convention. | ||
| - The controller (`index.ts`) correctly passes `backToListText: content.backToList`, so the template variable binding remains valid after the content change. | ||
| - All 637 unit tests pass without modification. | ||
|
|
||
| --- | ||
|
|
||
| ## Test Coverage Assessment | ||
|
|
||
| - **Unit tests:** Adequate. The `index.test.ts` for `audit-log-detail` has good coverage across the GET handler paths (no ID, not found, found with/without details, Welsh locale, role types, long details). The change does not require new test cases — existing tests exercise the changed code path. The suggestion above to tighten the `backToListText` assertion is optional. | ||
| - **E2E tests:** The `audit-log-viewer.spec.ts` file exists but the entire `test.describe` block is wrapped in `test.describe.skip(...)`. This means no E2E coverage runs for the audit log viewer at all, including the scenarios listed in the ticket's test requirements (back link text, date filter row layout). This is a pre-existing gap, not introduced by this ticket. A follow-up ticket to unskip or rewrite the E2E spec (once the SSO test environment is available) would close this gap. | ||
| - **Accessibility tests:** Covered inside the skipped E2E spec. No automated accessibility regression is introduced by this change; the removed CSS rule and cleaned template are neutral from an accessibility perspective. | ||
|
|
||
| --- | ||
|
|
||
| ## Acceptance Criteria Verification | ||
|
|
||
| - [x] Back link reads "Back" (English): `en.ts` line 44 updated to `"Back"`, passed through `backToListText` in controller and rendered in template. | ||
| - [x] Back link reads "Yn ôl" (Welsh): `cy.ts` line 44 updated to `"Yn ôl"`, correctly selected when `locale === "cy"`. | ||
| - [x] Date filter Day/Month/Year on one row: CSS rule removed from `dashboard.scss`; `formGroup: { classes: "app-date-filter-narrow" }` removed from `audit-log-list/index.njk`. The `govukDateInput` macro renders Day/Month/Year on a single flex row by default. | ||
|
|
||
| --- | ||
|
|
||
| ## Overall Assessment | ||
|
|
||
| APPROVED. All acceptance criteria are met, the implementation follows the content-file-driven pattern correctly, no regressions are introduced, and all tests pass. Address the test assertion suggestion (item 1 above) if the team wants tighter regression protection on the content values; it is not a blocker. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # Tasks: #467 Audit Log View Formatting Fixes | ||
|
|
||
| ## Implementation Tasks | ||
|
|
||
| - [x] In `libs/system-admin-pages/src/pages/audit-log-detail/en.ts` (line 44), change `backToList` value from `"Back to audit log list"` to `"Back"` | ||
| - [x] In `libs/system-admin-pages/src/pages/audit-log-detail/cy.ts` (line 44), change `backToList` value from `"Yn ôl i restr log archwilio"` to `"Yn ôl"` | ||
| - [x] In `libs/system-admin-pages/src/assets/css/dashboard.scss` (lines 43–45), remove the `.app-date-filter-narrow .govuk-date-input__item:last-child { padding-top: 10px; }` rule | ||
| - [x] In `libs/system-admin-pages/src/pages/audit-log-list/index.njk` (lines 99–101), remove the `formGroup: { classes: "app-date-filter-narrow" }` option from the `govukDateInput` call | ||
| - [x] Run `yarn test` in the `libs/system-admin-pages` workspace and confirm all existing tests pass |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
Isword-break: break-worddeprecated in current CSS specs, and isoverflow-wrap: anywherethe recommended replacement?💡 Result:
Yes, the word-break: break-word value is deprecated in current CSS specifications [1][2]. It was historically used as a legacy alias to prevent text from overflowing its container, but it is no longer considered appropriate for new development [3][4]. The recommended replacement is overflow-wrap: anywhere [2][4]. While both word-break: break-word and overflow-wrap: anywhere are designed to prevent long, unbreakable strings (like long words or URLs) from overflowing their containers, overflow-wrap: anywhere is the modern, standards-compliant way to achieve this behavior [4]. Using overflow-wrap: anywhere is preferred because it correctly accounts for soft wrap opportunities when calculating min-content intrinsic sizes, which helps ensure elements like inline-blocks or flex items behave predictably [4][5]. In contrast, the deprecated word-break: break-word acts as an alias for word-break: normal combined with overflow-wrap: anywhere, but it lacks the consistent, standardized behavior defined by modern CSS Text specifications [1][6][4].
Citations:
Replace
word-break: break-wordwithoverflow-wrap.word-break: break-wordis a legacy/deprecated value; useoverflow-wrapinstead for consistent cross-engine wrapping.Suggested patch
.audit-log-layout__word-break { - word-break: break-word; + overflow-wrap: anywhere; + word-break: normal; }📝 Committable suggestion
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 63-63: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)