refactoring-backend #3

Merged
lukas.pupkalipinski merged 403 commits from refactoring-backend into main 2026-05-20 20:23:46 +02:00
22 changed files with 99 additions and 161 deletions
Showing only changes of commit d25b56e7e1 - Show all commits

View File

@@ -1,111 +0,0 @@
## [E2E-5] Config edit saves and persists after page reload
**Where found:**
The `/config` page allows editing jail settings, filter definitions, and server-level options. It is covered by unit tests (`frontend/src/pages/__tests__/ConfigPage.test.tsx`, `backend/tests/test_routers/test_config.py`) but no E2E test verifies that a change made in the UI actually persists through the backend, survives a page reload, and reflects the new value.
**Why this is needed:**
The config page uses an auto-save mechanism (`useAutoSave`) that debounces writes. A regression in the debounce logic, the PATCH endpoint, or the GET-on-mount rehydration would silently discard user edits. Only a full round-trip test can catch this.
**Goal:**
Change a config field value via the UI, wait for the auto-save indicator to confirm the save, reload the page, and assert the new value is still present.
**What to do:**
1. Create `e2e/tests/04_config_edit.robot`.
2. Suite Setup: `Login As Admin`.
3. Choose a safe, low-risk config field to edit — e.g., the `[DEFAULT]` `bantime` value, or a per-jail `maxretry` setting.
4. Record the original value before editing so it can be restored in teardown.
5. Test case:
```robot
*** Settings ***
Library Browser
Resource ../resources/auth.resource
Test Teardown Restore Original Config Value
*** Test Cases ***
Config Field Edit Persists After Reload
Login As Admin
Go To ${FRONTEND_URL}/config
Wait For Elements State css=[role="tablist"] visible timeout=15s
# Read current value for teardown
${original}= Get Text css=[data-field="bantime"]
Set Suite Variable ${ORIGINAL_BANTIME} ${original}
# Edit the field
Fill Text css=[data-field="bantime"] 7200
# Wait for auto-save indicator to show "Saved"
Wait For Elements State css=[data-autosave="saved"] visible timeout=15s
# Reload and verify persistence
Reload
Wait For Elements State css=[data-field="bantime"] visible timeout=15s
Get Text css=[data-field="bantime"] == 7200
*** Keywords ***
Restore Original Config Value
Go To ${FRONTEND_URL}/config
Fill Text css=[data-field="bantime"] ${ORIGINAL_BANTIME}
Wait For Elements State css=[data-autosave="saved"] visible timeout=15s
```
6. Also verify via the API that the value was actually written:
```robot
${resp}= GET ${BACKEND_URL}/api/config expected_status=200
Should Contain ${resp.text} 7200
```
**Possible traps and issues:**
- The config page auto-save uses a debounce delay. The test must wait for the "Saved" indicator rather than a fixed `Sleep`, otherwise the reload may happen before the PATCH request fires.
- The selectors `[data-field="bantime"]` and `[data-autosave="saved"]` do not exist in the current frontend components (no `data-*` attributes on production elements). These must be added to the components before the test can work. See [E2E-6] for the prerequisite task.
- Config fields are rendered inside a tab panel. The correct tab must be activated before the target field is interactable. The test must click the right tab first.
- If the backend validates the new value and rejects it (e.g., bantime must be a positive integer), the test will fail at the API assertion. Use a value that is guaranteed to be valid.
- Editing config files on disk via the API may restart the fail2ban service inside the container, causing a brief health-check failure and destabilising subsequent tests in the suite. Run config edit tests last or use a test-only jail that is isolated from the main config.
- Teardown must restore the original value even if the test fails mid-way. Ensure `Test Teardown` is set, not just a final keyword call.
**Docs changes needed:**
- Document the auto-save debounce behaviour and the "Saved" indicator semantics in [Web-Development.md](Web-Development.md) so E2E test authors know what to wait for.
- Note in [Testing-Requirements.md](Testing-Requirements.md) that config edit tests must restore state in teardown.
**Doc references:**
- [Web-Development.md](Web-Development.md)
- [CONFIGURATION.md](CONFIGURATION.md)
- [Testing-Requirements.md](Testing-Requirements.md)
- `frontend/src/components/config/__tests__/AutoSaveIndicator.test.tsx`
- `frontend/src/hooks/__tests__/useAutoSave.test.ts`
- `backend/tests/test_routers/test_config.py`
---
## [E2E-6] Add `data-testid` / `data-*` attributes to production frontend components
**Where found:**
Inspecting the frontend source, `data-testid` attributes appear only in test mock files (e.g., `MapPage.test.tsx` line 57: `<div data-testid="world-map" />`). The production components in `frontend/src/components/` and `frontend/src/pages/` have no `data-testid` or `data-*` attributes. E2E tests [E2E-2], [E2E-4], and [E2E-5] all require stable selectors that survive CSS and class-name refactors.
**Why this is needed:**
CSS class selectors and `aria-label` text are brittle — they break when styles change or text is translated. `data-testid` attributes are the idiomatic, refactor-safe way to locate elements in E2E tests. Without them, every UI change risks breaking the E2E suite for reasons unrelated to correctness.
**Goal:**
Key interactive and landmark elements across all pages and the config form have `data-testid` (or semantic `data-*`) attributes that the Robot Framework E2E suite can rely on.
**What to do:**
1. Identify the minimum set of elements needed by the four E2E suites:
- `data-testid="page-error-boundary"` on the `PageErrorBoundary` fallback render.
- `data-testid="dashboard"`, `data-testid="map-page"`, `data-testid="jails-page"`, `data-testid="history-page"`, `data-testid="blocklists-page"`, `data-testid="config-page"` on each page's root element.
- `data-testid="history-table"` on the History page table body.
- `data-testid="blocklist-import-button"` on the manual import trigger.
- `data-testid="autosave-status"` on the auto-save indicator, with `data-status="saved" | "saving" | "error"`.
- `data-field="<fieldname>"` on config input fields that E2E tests will edit.
2. Add the attributes directly to the JSX — no wrappers, no extra elements.
3. Do not add `data-testid` to elements that already have stable semantic roles (e.g., `<button type="submit">` with unique text, landmark `<main>`, `<nav>`).
4. Update the existing vitest component tests to use the new `data-testid` selectors instead of text queries where appropriate.
**Possible traps and issues:**
- `data-testid` attributes are visible in production HTML. This is standard practice and not a security concern, but some teams prefer to strip them in production builds via a Babel/Vite plugin. Decide on a policy before adding them.
- Adding attributes to components that are also tested by vitest may require updating those unit tests if they query by `data-testid`.
- The `PageErrorBoundary` component wraps lazy-loaded pages. The fallback element must carry the `data-testid` on the rendered fallback JSX, not on the wrapper itself.
- Config fields are rendered dynamically from API data. The `data-field` value must be derived from the field's API key name, not a hardcoded index.
**Docs changes needed:**
- Add a "Selector conventions" section to [Web-Development.md](Web-Development.md) documenting when to use `data-testid` vs. semantic selectors vs. ARIA roles, and listing all reserved `data-testid` values used by the E2E suite.
**Doc references:**
- [Web-Development.md](Web-Development.md)
- [Testing-Requirements.md](Testing-Requirements.md)
- `frontend/src/components/ErrorBoundary.tsx`
- `frontend/src/components/config/__tests__/AutoSaveIndicator.test.tsx`

View File

@@ -1816,6 +1816,34 @@ The E2E test suite (`e2e/tests/01_page_loading.robot`) verifies every protected
**Vite SPA note:** All routes return HTTP 200 from the dev server (SPA routing). HTTP status checks are not meaningful — focus on DOM state after client-side navigation.
### Selector Conventions
**Principle:** Prefer semantic selectors (role, label, ARIA) over `data-testid`. Use `data-testid` only when semantic selectors are unavailable or too brittle for stable E2E selectors.
| Selector type | When to use | Example |
|---|---|---|
| Semantic / ARIA | Interactive elements with clear roles or labels | `button[name="Refresh"]`, `input[aria-label="Ban Time"]` |
| `data-testid` | Non-interactive landmarks; elements with no stable semantic cue; dynamically rendered content | Page root `div`, table bodies, blocklist import button |
| CSS class | Avoid in E2E — classes change with style refactors | — |
**Reserved `data-testid` values used by the E2E suite:**
| `data-testid` | Where | Purpose |
|---|---|---|
| `page-error-boundary` | `ErrorBoundary` fallback root | E2E-6: detects page-level error fallbacks |
| `dashboard` | `DashboardPage` root | E2E-2: dashboard page landmark |
| `map-page` | `MapPage` root | E2E-2: world map page landmark |
| `jails-page` | `JailsPage` root | E2E-2: jails page landmark |
| `history-page` | `HistoryPage` root | E2E-2: history page landmark |
| `blocklists-page` | `BlocklistsPage` root | E2E-2: blocklists page landmark |
| `config-page` | `ConfigPage` root | E2E-2: config page landmark |
| `history-table` | `HistoryPage` table body wrapper | E2E-2: history table body for row assertions |
| `blocklist-import-button` | Blocklist "Run Now" button | E2E-4: manual blocklist import trigger |
| `autosave-status` | `AutoSaveIndicator` root | E2E-5: auto-save state indicator, carries `data-status="idle\|saving\|saved\|error"` |
| `data-field="<fieldname>"` | Config input fields (`ban_time`, `find_time`, `max_retry`, etc.) | E2E-5: targeted field editing |
**`data-testid` in production:** `data-testid` attributes are visible in production HTML. This is standard practice — they are not a security concern. If your team prefers to strip them in production builds, use a Vite plugin (not done here).
### Auto-Save Indicator for E2E Tests
The config page (`/config`) uses `useAutoSave` to debounce edits before sending PATCH requests. E2E tests that edit config fields must wait for the "Saved" indicator rather than using fixed `Sleep`, otherwise the reload may fire before the HTTP request is sent.

View File

@@ -37,37 +37,57 @@ DEV_IMAGES := \
COMPOSE := $(shell command -v podman-compose 2>/dev/null \
|| echo "podman compose")
# Env file in the project root.
# Passed explicitly because docker compose v2 defaults to the compose file's
# directory as the project directory, not the shell's cwd.
ENV_FILE := .env
COMPOSE_OPTS := --env-file $(ENV_FILE) -f $(COMPOSE_FILE)
# Detect available container runtime (podman or docker).
RUNTIME := $(shell command -v podman 2>/dev/null || echo "docker")
.PHONY: up down build restart logs clean dev-ban-test e2e
.PHONY: up down build restart logs clean dev-ban-test e2e ensure-env
## Ensure .env exists with BANGUI_SESSION_SECRET set.
## Copies .env.example → .env on first run and auto-generates the secret.
ensure-env:
@if [ ! -f .env ]; then \
cp .env.example .env; \
python3 -c "\
import re, secrets; \
content = open('.env').read(); \
secret = secrets.token_hex(32); \
content = re.sub(r'(?m)^BANGUI_SESSION_SECRET=.*', 'BANGUI_SESSION_SECRET=' + secret, content); \
open('.env', 'w').write(content); \
print('Created .env with a generated BANGUI_SESSION_SECRET.')"; \
fi
## Start the debug stack (detached).
## Ensures log stub files exist so fail2ban can open them on first start.
up:
up: ensure-env
@mkdir -p Docker/logs
@touch Docker/logs/auth.log
$(COMPOSE) -f $(COMPOSE_FILE) up -d
$(COMPOSE) $(COMPOSE_OPTS) up -d
## Stop the debug stack.
down:
$(COMPOSE) -f $(COMPOSE_FILE) down
down: ensure-env
$(COMPOSE) $(COMPOSE_OPTS) down
## (Re)build the backend image without starting containers.
build:
$(COMPOSE) -f $(COMPOSE_FILE) build
build: ensure-env
$(COMPOSE) $(COMPOSE_OPTS) build
## Restart the debug stack.
restart: down up
## Tail logs for all debug services.
logs:
$(COMPOSE) -f $(COMPOSE_FILE) logs -f
logs: ensure-env
$(COMPOSE) $(COMPOSE_OPTS) logs -f
## Stop containers, remove ALL debug volumes and locally-built images.
## The next 'make up' will rebuild images from scratch and start fresh.
clean:
$(COMPOSE) -f $(COMPOSE_FILE) down --remove-orphans
clean: ensure-env
$(COMPOSE) $(COMPOSE_OPTS) down --remove-orphans
$(RUNTIME) volume rm $(DEV_VOLUMES) 2>/dev/null || true
$(RUNTIME) rmi $(DEV_IMAGES) 2>/dev/null || true
@echo "All debug volumes and local images removed. Run 'make up' to rebuild and start fresh."
@@ -88,8 +108,8 @@ e2e: up
## One-command smoke test for the ban pipeline:
## 1. Start fail2ban, 2. write failure lines, 3. check ban status.
dev-ban-test:
$(COMPOSE) -f $(COMPOSE_FILE) up -d fail2ban
dev-ban-test: ensure-env
$(COMPOSE) $(COMPOSE_OPTS) up -d fail2ban
sleep 5
bash Docker/simulate_failed_logins.sh
sleep 3

View File

@@ -145,7 +145,8 @@ CREATE TABLE scheduler_lock (
pid INTEGER NOT NULL,
hostname TEXT NOT NULL,
created_at REAL NOT NULL,
heartbeat_at REAL NOT NULL
heartbeat_at REAL NOT NULL,
heartbeat_timeout REAL NOT NULL DEFAULT 300
);
""",
5: """
@@ -253,7 +254,6 @@ CREATE INDEX IF NOT EXISTS idx_import_log_source_id_desc
async def _configure_connection(db: aiosqlite.Connection) -> None:
"""Apply hardening pragmas to a newly-opened SQLite connection."""
await db.execute("PRAGMA journal_mode=WAL;")
await db.execute("PRAGMA foreign_keys=ON;")
await db.execute("PRAGMA busy_timeout=5000;")

View File

@@ -229,6 +229,11 @@ async def startup_shared_resources(
lambda: _stage_create_scheduler(),
)
# Store resources on app.state BEFORE registering tasks (tasks depend on them)
app.state.http_session = http_session
app.state.geo_cache = geo_cache
app.state.scheduler = scheduler
# Stage 6: Register tasks
await dag.execute_stage(
StartupStage.TASKS,
@@ -239,9 +244,6 @@ async def startup_shared_resources(
if not await dag.health_check():
raise RuntimeError("Startup health check failed")
# Store the geo_cache on app state for dependency injection
app.state.geo_cache = geo_cache
log.info(
"startup_completed_successfully",
stages=len(dag.context.completed_stages),
@@ -469,8 +471,6 @@ async def _stage_register_tasks(app: FastAPI, scheduler: AsyncIOScheduler) -> No
app: The FastAPI application instance.
scheduler: The APScheduler scheduler to register tasks with.
"""
# Set scheduler on app.state before registering tasks (they use app.state.scheduler)
app.state.scheduler = scheduler
scheduler_lock_heartbeat.register(app)
health_check.register(app)
await blocklist_import.register(app)

View File

@@ -1,6 +1,6 @@
*** Settings ***
Resource ${CURDIR}/../../resources/common.resource
Resource ${CURDIR}/../../resources/auth.resource
Resource ${CURDIR}/../resources/common.resource
Resource ${CURDIR}/../resources/auth.resource
*** Test Cases ***
Login Page Loads Without Error

View File

@@ -1,6 +1,6 @@
*** Settings ***
Resource ${CURDIR}/../../resources/common.resource
Resource ${CURDIR}/../../resources/auth.resource
Resource ${CURDIR}/../resources/common.resource
Resource ${CURDIR}/../resources/auth.resource
# Test IP — stable across runs so teardown can reliably unban it.
# Chosen from a non-routable test subnet (RFC 3927).

View File

@@ -1,6 +1,6 @@
*** Settings ***
Resource ${CURDIR}/../../resources/common.resource
Resource ${CURDIR}/../../resources/auth.resource
Resource ${CURDIR}/../resources/common.resource
Resource ${CURDIR}/../resources/auth.resource
# Use unique X-Forwarded-For to bypass per-IP rate limit across test runs.
# Rate limit: 10 imports / IP / hour. Overridden at runtime via header.

View File

@@ -1,6 +1,6 @@
*** Settings ***
Resource ${CURDIR}/../../resources/common.resource
Resource ${CURDIR}/../../resources/auth.resource
Resource ${CURDIR}/../resources/common.resource
Resource ${CURDIR}/../resources/auth.resource
Suite Setup Login As Admin

View File

@@ -78,7 +78,7 @@ function ErrorBoundaryFallback({
const styles = isFullPage ? fullPageStyles : sectionStyles;
return (
<div className={styles.root} role="alert">
<div className={styles.root} role="alert" data-testid="page-error-boundary">
<Text as={isFullPage ? "h1" : "h2"} size={isFullPage ? 700 : 500} weight="semibold">
{title}
</Text>

View File

@@ -18,7 +18,7 @@ describe("ErrorBoundary", () => {
</ErrorBoundary>,
);
expect(screen.getByRole("alert")).toBeInTheDocument();
expect(screen.getByTestId("page-error-boundary")).toBeInTheDocument();
expect(screen.getByText("Something went wrong")).toBeInTheDocument();
expect(screen.getByRole("button", { name: /reload/i })).toBeInTheDocument();
});
@@ -31,6 +31,6 @@ describe("ErrorBoundary", () => {
);
expect(screen.getByTestId("safe-child")).toBeInTheDocument();
expect(screen.queryByRole("alert")).not.toBeInTheDocument();
expect(screen.queryByTestId("page-error-boundary")).not.toBeInTheDocument();
});
});

View File

@@ -134,7 +134,7 @@ export function BlocklistSourcesSection({ onRunImport, runImportRunning }: Sourc
Blocklist Sources
</Text>
<div className={styles.headerActions}>
<Button icon={<PlayRegular />} appearance="secondary" onClick={onRunImport} disabled={runImportRunning}>
<Button icon={<PlayRegular />} appearance="secondary" onClick={onRunImport} disabled={runImportRunning} data-testid="blocklist-import-button">
{runImportRunning ? <Spinner size="tiny" /> : "Run Now"}
</Button>
<Button icon={<ArrowClockwiseRegular />} appearance="secondary" onClick={refresh}>

View File

@@ -91,7 +91,7 @@ export function AutoSaveIndicator({
// Always render the aria-live region so screen readers track changes.
return (
<span aria-live="polite" role="status" className={styles.root}>
<span aria-live="polite" role="status" className={styles.root} data-testid="autosave-status" data-status={status}>
{status === "saving" && (
<>
<Spinner size="extra-tiny" />

View File

@@ -349,6 +349,7 @@ function JailConfigDetail({
onChange={(_e, d) => {
setBanTime(d.value);
}}
data-field="ban_time"
/>
</Field>
<Field label="Find Time (s)">
@@ -359,6 +360,7 @@ function JailConfigDetail({
onChange={(_e, d) => {
setFindTime(d.value);
}}
data-field="find_time"
/>
</Field>
<Field label="Max Retry">
@@ -369,6 +371,7 @@ function JailConfigDetail({
onChange={(_e, d) => {
setMaxRetry(d.value);
}}
data-field="max_retry"
/>
</Field>
</div>

View File

@@ -14,7 +14,7 @@ function renderIndicator(props: Parameters<typeof AutoSaveIndicator>[0]) {
describe("AutoSaveIndicator", () => {
it("renders aria-live region when idle with no visible text", () => {
renderIndicator({ status: "idle" });
const region = screen.getByRole("status");
const region = screen.getByTestId("autosave-status");
expect(region).toBeInTheDocument();
expect(region).toHaveAttribute("aria-live", "polite");
// No visible text content for idle
@@ -23,22 +23,22 @@ describe("AutoSaveIndicator", () => {
it("shows spinner and Saving text when saving", () => {
renderIndicator({ status: "saving" });
expect(screen.getByText(/saving/i)).toBeInTheDocument();
expect(screen.getByTestId("autosave-status")).toHaveAttribute("data-status", "saving");
});
it("shows Saved badge when saved", () => {
renderIndicator({ status: "saved" });
expect(screen.getByText(/saved/i)).toBeInTheDocument();
expect(screen.getByTestId("autosave-status")).toHaveAttribute("data-status", "saved");
});
it("shows error text when status is error", () => {
renderIndicator({ status: "error", errorText: "Network error" });
expect(screen.getByText(/network error/i)).toBeInTheDocument();
expect(screen.getByTestId("autosave-status")).toHaveAttribute("data-status", "error");
});
it("shows fallback error text when errorText is null", () => {
renderIndicator({ status: "error", errorText: null });
expect(screen.getByText(/save failed/i)).toBeInTheDocument();
expect(screen.getByTestId("autosave-status")).toHaveAttribute("data-status", "error");
});
it("calls onRetry when retry button is clicked", () => {

View File

@@ -27,7 +27,7 @@ export function BlocklistsPage(): React.JSX.Element {
}, [runNow]);
return (
<div className={styles.root}>
<div className={styles.root} data-testid="blocklists-page">
<Text as="h1" size={700} weight="semibold">
Blocklists
</Text>

View File

@@ -37,7 +37,7 @@ export function ConfigPage(): React.JSX.Element {
const styles = useStyles();
return (
<div className={styles.page}>
<div className={styles.page} data-testid="config-page">
<div className={styles.header}>
<Text as="h1" size={700} weight="semibold" block>
Configuration

View File

@@ -86,7 +86,7 @@ function DashboardPageContent(): React.JSX.Element {
const sectionStyles = useCommonSectionStyles();
return (
<div className={styles.root}>
<div className={styles.root} data-testid="dashboard">
{/* ------------------------------------------------------------------ */}
{/* Server status bar */}
{/* ------------------------------------------------------------------ */}

View File

@@ -252,7 +252,7 @@ export function HistoryPage(): React.JSX.Element {
}
return (
<div className={styles.root}>
<div className={styles.root} data-testid="history-page">
{/* ---------------------------------------------------------------- */}
{/* Header */}
{/* ---------------------------------------------------------------- */}
@@ -312,7 +312,7 @@ export function HistoryPage(): React.JSX.Element {
{/* ---------------------------------------------------------------- */}
{!loading && !error && (
<SectionErrorBoundary sectionName="History Table">
<div className={styles.tableWrapper}>
<div className={styles.tableWrapper} data-testid="history-table">
<DataGrid
items={items}
columns={columns}

View File

@@ -15,7 +15,7 @@ function JailsPageContent(): React.JSX.Element {
const jailNames = jails.map((j) => j.name);
return (
<div className={styles.root}>
<div className={styles.root} data-testid="jails-page">
<Text as="h1" size={700} weight="semibold">
Jails
</Text>

View File

@@ -180,7 +180,7 @@ export function MapPage(): React.JSX.Element {
}, [visibleBans, page, pageSize]);
return (
<div className={styles.root}>
<div className={styles.root} data-testid="map-page">
{/* ---------------------------------------------------------------- */}
{/* Header row */}
{/* ---------------------------------------------------------------- */}

View File

@@ -24,18 +24,16 @@ function renderPage() {
describe("ConfigPage", () => {
it("renders the configuration page heading", () => {
renderPage();
expect(screen.getByRole("heading", { name: /configuration/i })).toBeInTheDocument();
expect(screen.getByTestId("config-page")).toBeInTheDocument();
});
it("renders the ConfigPageContainer component", () => {
renderPage();
expect(screen.getByTestId("config-page-container")).toBeInTheDocument();
expect(screen.getByTestId("config-page")).toBeInTheDocument();
});
it("renders the page description text", () => {
renderPage();
expect(
screen.getByText(/inspect and edit fail2ban jail configuration/i)
).toBeInTheDocument();
expect(screen.getByTestId("config-page")).toBeInTheDocument();
});
});