Fix ActivateJailDialog blocking logic and mypy false positive
Two frontend bugs and one mypy false positive fixed: - ActivateJailDialog: Activate button was never disabled when blockingIssues.length > 0 (missing condition in disabled prop). - ActivateJailDialog: handleConfirm called onActivated() even when the backend returned active=false (blocked activation). Dialog now stays open and shows result.message instead. - config.py: Settings() call flagged by mypy --strict because pydantic-settings loads required fields from env vars at runtime; suppressed with a targeted type: ignore[call-arg] comment. Tests: added ActivateJailDialog.test.tsx (5 tests covering button state, backend-rejection handling, success path, and crash detection callback).
This commit is contained in:
131
Docs/Tasks.md
131
Docs/Tasks.md
@@ -331,3 +331,134 @@ The `deactivate_jail` endpoint in `backend/app/routers/config.py` is inconsisten
|
||||
3. **Manual test with running server:**
|
||||
- Go to `/config`, find an active jail and click "Deactivate".
|
||||
- Immediately navigate to the Dashboard — "Active Jails" count should already reflect the reduced count without any delay.
|
||||
|
||||
---
|
||||
|
||||
## Task 7 — Fix ActivateJailDialog not honouring backend rejection and mypy false positive
|
||||
|
||||
**Status:** in progress
|
||||
|
||||
### Problem description
|
||||
|
||||
Two independent bugs were introduced during Tasks 5–6:
|
||||
|
||||
**Bug 1 — "Activate" button is never disabled on validation errors (frontend)**
|
||||
|
||||
In `frontend/src/components/config/ActivateJailDialog.tsx`, Task 5 Part B set:
|
||||
|
||||
```tsx
|
||||
const blockingIssues = validationIssues; // all issues block activation
|
||||
```
|
||||
|
||||
but the "Activate" `<Button>` `disabled` prop was never updated to include `blockingIssues.length > 0`:
|
||||
|
||||
```tsx
|
||||
disabled={submitting || validating} // BUG: missing `|| blockingIssues.length > 0`
|
||||
```
|
||||
|
||||
The pre-validation error message renders correctly, but the button stays clickable. A user can press "Activate" despite seeing a red error — the backend will refuse (returning `active=false`) but the UX is broken and confusing.
|
||||
|
||||
**Bug 2 — Dialog closes and fires `onActivated()` even when backend rejects activation (frontend)**
|
||||
|
||||
`handleConfirm`'s `.then()` handler never inspects `result.active`. When the backend blocks activation and returns `{ active: false, message: "...", validation_warnings: [...] }`, the frontend still:
|
||||
|
||||
1. Calls `setValidationWarnings(result.validation_warnings)` — sets warnings in state.
|
||||
2. Immediately calls `resetForm()` — which **clears** the newly-set warnings.
|
||||
3. Calls `onActivated()` — which triggers the parent to refresh the jail list (and may close the dialog).
|
||||
|
||||
The user sees the dialog briefly appear to succeed, the parent refreshes, but the jail never activated.
|
||||
|
||||
**Bug 3 — mypy strict false positive in `config.py`**
|
||||
|
||||
`get_settings()` calls `Settings()` without arguments. mypy strict mode flags this as:
|
||||
```
|
||||
backend/app/config.py:88: error: Missing named argument "session_secret" for "Settings" [call-arg]
|
||||
```
|
||||
This is a known pydantic-settings limitation: the library loads required fields from environment variables at runtime, which mypy cannot see statically. A targeted suppression with an explanatory comment is the correct fix.
|
||||
|
||||
### What to do
|
||||
|
||||
#### Part A — Disable "Activate" button when blocking issues are present (frontend)
|
||||
|
||||
**File:** `frontend/src/components/config/ActivateJailDialog.tsx`
|
||||
|
||||
Find the "Activate" `<Button>` near the bottom of the returned JSX and change its `disabled` prop:
|
||||
|
||||
```tsx
|
||||
// Before:
|
||||
disabled={submitting || validating}
|
||||
|
||||
// After:
|
||||
disabled={submitting || validating || blockingIssues.length > 0}
|
||||
```
|
||||
|
||||
#### Part B — Handle `active=false` response from backend (frontend)
|
||||
|
||||
**File:** `frontend/src/components/config/ActivateJailDialog.tsx`
|
||||
|
||||
In `handleConfirm`'s `.then()` callback, add a check for `result.active` before calling `resetForm()` and `onActivated()`:
|
||||
|
||||
```tsx
|
||||
.then((result) => {
|
||||
if (!result.active) {
|
||||
// Backend rejected the activation (e.g. missing logpath).
|
||||
// Show the server's message and keep the dialog open.
|
||||
setError(result.message);
|
||||
return;
|
||||
}
|
||||
if (result.validation_warnings.length > 0) {
|
||||
setValidationWarnings(result.validation_warnings);
|
||||
}
|
||||
resetForm();
|
||||
if (!result.fail2ban_running) {
|
||||
onCrashDetected?.();
|
||||
}
|
||||
onActivated();
|
||||
})
|
||||
```
|
||||
|
||||
#### Part C — Fix mypy false positive (backend)
|
||||
|
||||
**File:** `backend/app/config.py`
|
||||
|
||||
Add a targeted `# type: ignore[call-arg]` with an explanatory comment to the `Settings()` call in `get_settings()`:
|
||||
|
||||
```python
|
||||
return Settings() # type: ignore[call-arg] # pydantic-settings populates required fields from env vars
|
||||
```
|
||||
|
||||
### Tests to add or update
|
||||
|
||||
**File:** `frontend/src/components/config/__tests__/ActivateJailDialog.test.tsx` (new file)
|
||||
|
||||
Write tests covering:
|
||||
|
||||
1. **`test_activate_button_disabled_when_blocking_issues`** — render the dialog with mocked `validateJailConfig` returning an issue with `field="logpath"`. Assert the "Activate" button is disabled.
|
||||
|
||||
2. **`test_activate_button_enabled_when_no_issues`** — render the dialog with mocked `validateJailConfig` returning no issues. Assert the "Activate" button is enabled after validation completes.
|
||||
|
||||
3. **`test_dialog_stays_open_when_backend_returns_active_false`** — mock `activateJail` to return `{ active: false, message: "Jail cannot be activated", validation_warnings: [], fail2ban_running: true, name: "test" }`. Click "Activate". Assert: (a) `onActivated` is NOT called; (b) the error message text appears.
|
||||
|
||||
4. **`test_dialog_calls_on_activated_when_backend_returns_active_true`** — mock `activateJail` to return `{ active: true, message: "ok", validation_warnings: [], fail2ban_running: true, name: "test" }`. Click "Activate". Assert `onActivated` is called once.
|
||||
|
||||
5. **`test_crash_detected_callback_fires_when_fail2ban_not_running`** — mock `activateJail` to return `active: true, fail2ban_running: false`. Assert `onCrashDetected` is called.
|
||||
|
||||
### Verification
|
||||
|
||||
1. Run frontend type check and lint:
|
||||
```bash
|
||||
cd frontend && npx tsc --noEmit && npx eslint src/components/config/ActivateJailDialog.tsx
|
||||
```
|
||||
Zero errors and zero warnings.
|
||||
|
||||
2. Run frontend tests:
|
||||
```bash
|
||||
cd frontend && npx vitest run src/components/config/__tests__/ActivateJailDialog
|
||||
```
|
||||
All 5 new tests pass.
|
||||
|
||||
3. Run mypy:
|
||||
```bash
|
||||
.venv/bin/mypy backend/app/ --strict
|
||||
```
|
||||
Zero errors.
|
||||
|
||||
@@ -85,4 +85,4 @@ def get_settings() -> Settings:
|
||||
A validated :class:`Settings` object. Raises :class:`pydantic.ValidationError`
|
||||
if required keys are absent or values fail validation.
|
||||
"""
|
||||
return Settings()
|
||||
return Settings() # type: ignore[call-arg] # pydantic-settings populates required fields from env vars
|
||||
|
||||
@@ -152,6 +152,13 @@ export function ActivateJailDialog({
|
||||
|
||||
activateJail(jail.name, overrides)
|
||||
.then((result) => {
|
||||
if (!result.active) {
|
||||
// Backend rejected the activation (e.g. missing logpath or filter).
|
||||
// Show the server's message and keep the dialog open so the user
|
||||
// can read the explanation without the dialog disappearing.
|
||||
setError(result.message);
|
||||
return;
|
||||
}
|
||||
if (result.validation_warnings.length > 0) {
|
||||
setValidationWarnings(result.validation_warnings);
|
||||
}
|
||||
@@ -336,7 +343,7 @@ export function ActivateJailDialog({
|
||||
<Button
|
||||
appearance="primary"
|
||||
onClick={handleConfirm}
|
||||
disabled={submitting || validating}
|
||||
disabled={submitting || validating || blockingIssues.length > 0}
|
||||
icon={submitting ? <Spinner size="tiny" /> : undefined}
|
||||
>
|
||||
{submitting ? "Activating and verifying…" : "Activate"}
|
||||
|
||||
@@ -0,0 +1,229 @@
|
||||
/**
|
||||
* Tests for ActivateJailDialog (Task 7).
|
||||
*
|
||||
* Covers:
|
||||
* - "Activate" button is disabled when pre-validation returns blocking issues.
|
||||
* - "Activate" button is enabled when validation passes.
|
||||
* - Dialog stays open and shows an error when the backend returns active=false.
|
||||
* - `onActivated` is called and dialog closes when backend returns active=true.
|
||||
* - `onCrashDetected` is called when fail2ban_running is false after activation.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { render, screen, waitFor } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
||||
import { ActivateJailDialog } from "../ActivateJailDialog";
|
||||
import type { InactiveJail, JailActivationResponse, JailValidationResult } from "../../../types/config";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Mocks
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
vi.mock("../../../api/config", () => ({
|
||||
activateJail: vi.fn(),
|
||||
validateJailConfig: vi.fn(),
|
||||
}));
|
||||
|
||||
import { activateJail, validateJailConfig } from "../../../api/config";
|
||||
|
||||
const mockActivateJail = vi.mocked(activateJail);
|
||||
const mockValidateJailConfig = vi.mocked(validateJailConfig);
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fixtures
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const baseJail: InactiveJail = {
|
||||
name: "airsonic-auth",
|
||||
filter: "airsonic-auth",
|
||||
actions: ["iptables-multiport"],
|
||||
port: "8080",
|
||||
logpath: ["/var/log/airsonic/airsonic.log"],
|
||||
bantime: "10m",
|
||||
findtime: "10m",
|
||||
maxretry: 5,
|
||||
ban_time_seconds: 600,
|
||||
find_time_seconds: 600,
|
||||
log_encoding: "auto",
|
||||
backend: "auto",
|
||||
date_pattern: null,
|
||||
use_dns: "warn",
|
||||
prefregex: "",
|
||||
fail_regex: ["Failed login.*from <HOST>"],
|
||||
ignore_regex: [],
|
||||
bantime_escalation: null,
|
||||
source_file: "/config/fail2ban/jail.d/airsonic-auth.conf",
|
||||
enabled: false,
|
||||
};
|
||||
|
||||
/** Successful activation response. */
|
||||
const successResponse: JailActivationResponse = {
|
||||
name: "airsonic-auth",
|
||||
active: true,
|
||||
message: "Jail activated successfully.",
|
||||
fail2ban_running: true,
|
||||
validation_warnings: [],
|
||||
};
|
||||
|
||||
/** Response when backend blocks activation (e.g. missing logpath). */
|
||||
const blockedResponse: JailActivationResponse = {
|
||||
name: "airsonic-auth",
|
||||
active: false,
|
||||
message: "Jail 'airsonic-auth' cannot be activated: logpath does not exist.",
|
||||
fail2ban_running: true,
|
||||
validation_warnings: ["logpath: /var/log/airsonic/airsonic.log does not exist"],
|
||||
};
|
||||
|
||||
/** Validation result with a logpath issue (should block the button). */
|
||||
const validationWithLogpathIssue: JailValidationResult = {
|
||||
jail_name: "airsonic-auth",
|
||||
valid: false,
|
||||
issues: [{ field: "logpath", message: "/var/log/airsonic/airsonic.log does not exist" }],
|
||||
};
|
||||
|
||||
/** Validation result with no issues. */
|
||||
const validationPassed: JailValidationResult = {
|
||||
jail_name: "airsonic-auth",
|
||||
valid: true,
|
||||
issues: [],
|
||||
};
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Helpers
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
interface DialogProps {
|
||||
jail?: InactiveJail | null;
|
||||
open?: boolean;
|
||||
onClose?: () => void;
|
||||
onActivated?: () => void;
|
||||
onCrashDetected?: () => void;
|
||||
}
|
||||
|
||||
function renderDialog({
|
||||
jail = baseJail,
|
||||
open = true,
|
||||
onClose = vi.fn(),
|
||||
onActivated = vi.fn(),
|
||||
onCrashDetected = vi.fn(),
|
||||
}: DialogProps = {}) {
|
||||
return render(
|
||||
<FluentProvider theme={webLightTheme}>
|
||||
<ActivateJailDialog
|
||||
jail={jail}
|
||||
open={open}
|
||||
onClose={onClose}
|
||||
onActivated={onActivated}
|
||||
onCrashDetected={onCrashDetected}
|
||||
/>
|
||||
</FluentProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Tests
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("ActivateJailDialog", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it("disables the Activate button when pre-validation returns blocking issues", async () => {
|
||||
mockValidateJailConfig.mockResolvedValue(validationWithLogpathIssue);
|
||||
|
||||
renderDialog();
|
||||
|
||||
// Wait for validation to complete and the error message to appear.
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/configuration errors detected/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
const activateBtn = screen.getByRole("button", { name: /^activate$/i });
|
||||
expect(activateBtn).toBeDisabled();
|
||||
});
|
||||
|
||||
it("enables the Activate button when validation passes", async () => {
|
||||
mockValidateJailConfig.mockResolvedValue(validationPassed);
|
||||
|
||||
renderDialog();
|
||||
|
||||
// Wait for validation spinner to disappear.
|
||||
await waitFor(() => {
|
||||
expect(screen.queryByText(/validating configuration/i)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
const activateBtn = screen.getByRole("button", { name: /^activate$/i });
|
||||
expect(activateBtn).not.toBeDisabled();
|
||||
});
|
||||
|
||||
it("keeps the dialog open and shows an error when backend returns active=false", async () => {
|
||||
mockValidateJailConfig.mockResolvedValue(validationPassed);
|
||||
mockActivateJail.mockResolvedValue(blockedResponse);
|
||||
|
||||
const onActivated = vi.fn();
|
||||
renderDialog({ onActivated });
|
||||
|
||||
// Wait for validation to finish.
|
||||
await waitFor(() => {
|
||||
expect(screen.queryByText(/validating configuration/i)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
const activateBtn = screen.getByRole("button", { name: /^activate$/i });
|
||||
await userEvent.click(activateBtn);
|
||||
|
||||
// The server's error message should appear.
|
||||
await waitFor(() => {
|
||||
expect(
|
||||
screen.getByText(/cannot be activated/i),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// onActivated must NOT have been called.
|
||||
expect(onActivated).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("calls onActivated when backend returns active=true", async () => {
|
||||
mockValidateJailConfig.mockResolvedValue(validationPassed);
|
||||
mockActivateJail.mockResolvedValue(successResponse);
|
||||
|
||||
const onActivated = vi.fn();
|
||||
renderDialog({ onActivated });
|
||||
|
||||
await waitFor(() => {
|
||||
expect(screen.queryByText(/validating configuration/i)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
const activateBtn = screen.getByRole("button", { name: /^activate$/i });
|
||||
await userEvent.click(activateBtn);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(onActivated).toHaveBeenCalledOnce();
|
||||
});
|
||||
});
|
||||
|
||||
it("calls onCrashDetected when fail2ban_running is false after activation", async () => {
|
||||
mockValidateJailConfig.mockResolvedValue(validationPassed);
|
||||
mockActivateJail.mockResolvedValue({
|
||||
...successResponse,
|
||||
fail2ban_running: false,
|
||||
});
|
||||
|
||||
const onActivated = vi.fn();
|
||||
const onCrashDetected = vi.fn();
|
||||
renderDialog({ onActivated, onCrashDetected });
|
||||
|
||||
await waitFor(() => {
|
||||
expect(screen.queryByText(/validating configuration/i)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
const activateBtn = screen.getByRole("button", { name: /^activate$/i });
|
||||
await userEvent.click(activateBtn);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(onCrashDetected).toHaveBeenCalledOnce();
|
||||
});
|
||||
expect(onActivated).toHaveBeenCalledOnce();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user