spec(service-credential-tester): add tasks (2 slices, each <=400 lines)
S1 backend: TestResult + test_callable + translate_connection_error + POST /api/services/test + 7 per-type routines + tests. S2 frontend: type + API fn + hook + shared ServiceTestPanel wired into CreateServiceDialog + Settings.tsx ServiceConfigEditor (per design source-finding). Each slice leaves pytest/npm build/npm lint green.
This commit is contained in:
Developer
@@ -0,0 +1,266 @@
|
||||
# SDD Tasks: Service Credential Tester
|
||||
|
||||
**Change:** `service-credential-tester`
|
||||
**Phase:** tasks
|
||||
**Date:** 2026-07-09
|
||||
|
||||
## Review Workload Forecast
|
||||
|
||||
| Field | Value |
|
||||
|-------|-------|
|
||||
| Estimated changed lines | ~550–700 (sum of two implementation slices) |
|
||||
| 400-line budget risk | Low–Medium |
|
||||
| Chained PRs recommended | Yes |
|
||||
| Suggested split | PR 1: backend endpoint + TestResult/test_callable/translate_connection_error + 7 per-type routines + tests → PR 2: frontend type + API fn + hook + ServiceTestPanel + wire into both surfaces + tests |
|
||||
| Delivery strategy | auto-chain |
|
||||
| Chain strategy | stacked-to-main |
|
||||
|
||||
```text
|
||||
Decision needed before apply: No
|
||||
Chained PRs recommended: Yes
|
||||
Chain strategy: stacked-to-main
|
||||
400-line budget risk: Low–Medium
|
||||
```
|
||||
|
||||
> Each slice individually lands under the 400-line review budget. Slices are ordered S1 → S2; S1 is independently shippable (backend endpoint works, frontend has no Test UI until S2). Per `openspec/config.yaml` rules, each slice leaves `npm run build` (tsc -b + vite build), `npm run lint`, and backend `pytest` green.
|
||||
|
||||
---
|
||||
|
||||
## Slice ordering rationale (critical)
|
||||
|
||||
**Slice 1 builds the backend endpoint + per-type test routines.** After S1:
|
||||
|
||||
- `POST /api/services/test` validates input (reuses `_validate_input`), dispatches to the definition's `test_callable`, and returns `{ok, detail, evidence}` — no persistence.
|
||||
- Each of 7 service types (qbittorrent, prometheus, alertmanager, jellyfin, authentik, ssh_tasks, nextcloud) has a `test_connection` routine; backups has `test_callable = None`.
|
||||
- `TestResult` dataclass + `translate_connection_error` shared helper live in `integrations/base.py`.
|
||||
- All backend tests are green: per-type mocked routines, endpoint dispatch, validation-first, no-persistence.
|
||||
- The frontend has NO Test UI yet — the endpoint is callable via API only. That's fine; S2 adds the UI.
|
||||
|
||||
**Slice 2 adds the frontend Test UI.** A shared `ServiceTestPanel` component (Test button + result pill + Save-anyway override + field-edit-clears-result) is wired into BOTH `CreateServiceDialog` (in `ServicesPage.tsx`) AND `ServiceConfigEditor` (in `Settings.tsx` — per the design source finding, NOT `ServicePage.tsx`). The panel is identical in both surfaces.
|
||||
|
||||
This ordering ensures the backend is proven (endpoint dispatches correctly, validation rejects malformed input, no-persistence holds) before any frontend churn.
|
||||
|
||||
---
|
||||
|
||||
## Source-finding corrections (read before applying)
|
||||
|
||||
The design (§0) surfaced deviations from the proposal/spec. Tasks below incorporate these corrections:
|
||||
|
||||
| Spec/proposal claim | Actual source reality | Correction in tasks |
|
||||
|---|---|---|
|
||||
| CT-115: "the edit dialog on `ServicePage.tsx`" | **`ServicePage.tsx` has no edit dialog.** Service editing lives in `Settings.tsx::ServiceConfigEditor` (master-detail panel, lines ~1435–1596). | Task 2.6 targets `Settings.tsx::ServiceConfigEditor`, NOT `ServicePage.tsx`. |
|
||||
| CT-109: "reuses the `test_machine_ssh` connect flow" | `test_machine_ssh` (settings.py:122–180) is a **router endpoint**, not a reusable function. But `build_ssh_client` in `task_runner.py` builds the client from a `ServiceRecord`. | Task 1.8 uses `build_ssh_client(store, ServiceRecord)` + `.connect()`, then applies the same error-translation patterns inline. It does NOT import or call `test_machine_ssh`. |
|
||||
| Proposal: "construct a `QbittorrentClient` from the config + decrypted secrets" | `QbittorrentClient.__init__` takes positional strings (`base_url, username, password, timeout`), not a config dict. | Task 1.4 extracts fields explicitly from `config`/`secrets` dicts. |
|
||||
| Proposal mentions `jellyseerr` as a distinct type | Active registry has no `jellyseerr` entry (merged into Jellyfin by `services-as-hub-ia`). | No jellyseerr test routine. The 7 remote types are: qbittorrent, prometheus, alertmanager, jellyfin, authentik, ssh_tasks, nextcloud. |
|
||||
|
||||
No proposal/spec scope change is required — the intent (per-type credential tester) holds. These corrections refine implementation details and the edit-surface target.
|
||||
|
||||
---
|
||||
|
||||
## Slice 1: Backend endpoint + test_callable + per-type routines + tests
|
||||
|
||||
**Goal:** Add `POST /api/services/test` that validates unsaved service input, dispatches to a per-type `test_callable`, and returns `{ok, detail, evidence}` without persisting. Each of 7 service types gets a `test_connection` routine; backups has no test.
|
||||
|
||||
**Satisfies:** CT-101, CT-102, CT-103, CT-104, CT-105, CT-106, CT-107, CT-108, CT-109, CT-110, CT-111, CT-112, CT-113, CT-119.
|
||||
|
||||
- [ ] **1.1 Add `TestResult` dataclass + `TestCallable` type alias + `test_callable` field on `ServiceDefinition` (CT-101, CT-103)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/base.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: none
|
||||
- Details: Add `@dataclass(frozen=True) class TestResult` with fields `ok: bool`, `detail: str`, `evidence: str | None = None`. Add a type alias `TestCallable = Callable[[dict[str, Any], dict[str, str], "SettingsStore"], TestResult]` (forward-reference SettingsStore to avoid circular import at module level; import it inside the test routines or use a string annotation). Add `test_callable: TestCallable | None = None` to `ServiceDefinition` — placed LAST (after `widget_kinds`) so the dataclass field-ordering with a default works. Import `Callable` from `typing`. Update `TYPE_CHECKING` guard if needed for the SettingsStore forward reference.
|
||||
|
||||
- [ ] **1.2 Add `translate_connection_error` shared helper (CT-104..CT-110)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/base.py` (modify, same file)
|
||||
- Lines: ~40
|
||||
- Dependencies: 1.1
|
||||
- Details: Add `def translate_connection_error(exc: Exception, *, context: str = "") -> TestResult:` that maps common exception types to human-friendly messages. Handle: `requests.HTTPError` with status 401/403 → auth message; `requests.ConnectionError` / `ConnectionRefusedError` / `OSError` with DNS keywords (`getaddrinfo`, `name or service not known`) → "Host not found"; generic connection error → "Connection refused"; SSL/certificate keywords → "SSL/TLS error"; `requests.Timeout` / `TimeoutError` / `asyncio.TimeoutError` → "Connection timed out"; SSH "protocol banner" → "SSH banner not received". Fallback: truncate message to 200 chars, prefix with context. The helper returns `TestResult(ok=False, detail=...)` always. Import `requests` and `asyncio` at the top of `base.py` (or inside the function to avoid circular imports — prefer top-level since `base.py` already may need `requests` for the type checks).
|
||||
|
||||
- [ ] **1.3 Add `test_connection` for qbittorrent (CT-104)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/qbittorrent.py` (modify)
|
||||
- Lines: ~25
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:` that constructs `QbittorrentClient(config["base_url"], secrets["username"], secrets["password"], timeout=config.get("timeout_seconds", 10))`, calls `client.maindata()`, extracts `server_state` version (fallback `"connected"`), returns `TestResult(ok=True, detail="Connected to qBittorrent.", evidence=version)`. Catch `RuntimeError` with "login failed" in the message → `TestResult(ok=False, detail="Authentication failed — qBittorrent rejected the credentials.")`. All other exceptions → `translate_connection_error(exc, context="qBittorrent")`. Import `QbittorrentClient` from `clients.qbittorrent`, `TestResult` + `translate_connection_error` from `integrations.base`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [ ] **1.4 Add `test_connection` for prometheus via Grafana gateway (CT-105)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/prometheus.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that issues `POST {grafana_url}/api/ds/query` with `Authorization: Bearer {grafana_api_key}`, body `{"queries": [{"datasource": {"uid": datasource_uid, "type": "prometheus"}, "expr": "up", "format": "time_series", "intervalMs": 15000, "maxDataPoints": 1, "refId": "A"}], "from": "now-1m", "to": "now"}`. Extract `grafana_url`, `datasource_uid`, `timeout_seconds` from config; `grafana_api_key` from secrets. Missing URL/key → `TestResult(ok=False, detail="...")` (no network). Success → `TestResult(ok=True, detail="Grafana gateway reachable.", evidence="Gateway reachable; datasource responded.")`. Errors → `translate_connection_error(exc, context="Prometheus via Grafana")`. Mirror `MetricSource._gateway_query` request shape exactly. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [ ] **1.5 Add `test_connection` for alertmanager (CT-106)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/alertmanager.py` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that GETs `{base_url}/api/v2/status` with optional `Authorization: Bearer {api_key}` (when secret present). Extract version from `versionInfo.version` (fallback `"connected"`). Success → `TestResult(ok=True, detail="Connected to Alertmanager.", evidence=version)`. Errors → `translate_connection_error(exc, context="Alertmanager")`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [ ] **1.6 Add `test_connection` for jellyfin (CT-107)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/jellyfin.py` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs `JellyfinClient(config["base_url"], secrets["api_key"], timeout=config.get("timeout_seconds", 10))` and calls `.users()`. Success → `TestResult(ok=True, detail="Connected to Jellyfin.", evidence=f"{len(users)} users")`. Errors → `translate_connection_error(exc, context="Jellyfin")`. Import `JellyfinClient` from `clients.jellyfin`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [ ] **1.7 Add `test_connection` for authentik (CT-108)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/authentik.py` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs `AuthentikClient(base_url=config["base_url"], api_token=secrets["api_token"], timeout=config.get("timeout_seconds", 10))` and calls `.users(page=1, page_size=1)` — the lightest directory probe. Success → `TestResult(ok=True, detail="Connected to Authentik.", evidence=...)`. Errors → `translate_connection_error(exc, context="Authentik")`. Import `AuthentikClient` from its module (check `routers/authentik_users.py::_build_client` for the exact import path + constructor signature). Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [ ] **1.8 Add `test_connection` for ssh_tasks reusing `build_ssh_client` (CT-109)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/ssh_tasks.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs a `ServiceRecord(id="", service_type="ssh_tasks", name="test", config=config, secrets=secrets, enabled=True)`, calls `build_ssh_client(store, service)` (from `services.task_runner`), then `.connect()` inside try/finally with `.close()`. Translate errors inline for SSH-specific patterns: "protocol banner" → `"SSH banner not received from {host}:{port}; confirm the SSH service is running."`; "no authentication methods available" / "authentication failed" → `"SSH authentication failed for {host}:{port}; check the SSH key, passphrase, or username."`. Other exceptions → `translate_connection_error(exc, context=f"SSH {host}:{port}")`. Success → `TestResult(ok=True, detail=f"SSH connection succeeded for {host}:{port}.", evidence=f"Connected to {host}:{port}")`. Import `build_ssh_client` from `media_library_viewer_api.services.task_runner` and `ServiceRecord` from `media_library_viewer_api.widgets.sources` (or define locally to avoid circular import — check). **Do NOT import or call `test_machine_ssh`** — it's a router endpoint, not a reusable function. Known-host recording is preserved automatically by `RemoteSSHClient.connect()`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [ ] **1.9 Add `test_connection` for nextcloud (CT-110)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/nextcloud.py` (modify)
|
||||
- Lines: ~15
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that GETs `{base_url}/status.php` (unauthenticated — `/status.php` is public). Extract `version` from the JSON response (fallback `"connected"`). Success → `TestResult(ok=True, detail="Connected to Nextcloud.", evidence=version)`. Errors → `translate_connection_error(exc, context="Nextcloud")`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [ ] **1.10 Confirm backups DEFINITION has `test_callable = None` (CT-111)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/backups.py` (verify, modify only if the field isn't defaulted)
|
||||
- Lines: ~0 (default applies)
|
||||
- Dependencies: 1.1
|
||||
- Details: `backups` should NOT gain a `test_connection` function. Its `DEFINITION` relies on the default `test_callable=None` from `ServiceDefinition`. Verify by reading the file; no code change expected unless the DEFINITION is constructed with explicit keyword args that omit `test_callable` (the default handles it). The endpoint returns `{ok: true, detail: "No connection test for this service type"}` automatically.
|
||||
|
||||
- [ ] **1.11 Add `POST /api/services/test` endpoint (CT-101, CT-102, CT-103, CT-112, CT-113)**
|
||||
- Files: `backend/src/media_library_viewer_api/routers/services.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: 1.1–1.10
|
||||
- Details: Add `@router.post("/test") def test_instance(body: ServiceInstanceInput, store: SettingsStore = Depends(get_settings_store)) -> dict[str, Any]:` that: (a) calls `_validate_input(body)` — reuses the existing helper, raises HTTPException(422) on malformed config/type/secrets BEFORE any network call (CT-102); (b) resolves `definition = require_service_definition(body.service_type)`; (c) if `definition.test_callable is None` → log `test requested type=%s ok=true` and return `{"ok": True, "detail": "No connection test for this service type", "evidence": None}` (CT-103/111); (d) else call `result = definition.test_callable(body.config, body.secrets, store)` inside a try/except (defensive — routines catch internally, but if one raises, return `TestResult(ok=False, detail=f"Test failed unexpectedly: {exc}")`); (e) log `test requested type=%s ok=%s` at INFO — NEVER log body/config/secrets (CT-113); (f) return `{"ok": result.ok, "detail": result.detail, "evidence": result.evidence}`. NO call to `store.upsert_service`, `store.update_setting`, or any persistence method (CT-112). Import `require_service_definition` from `integrations.registry` (already imported or adjacent).
|
||||
|
||||
- [ ] **1.12 Add backend tests for `translate_connection_error` (CT-119)**
|
||||
- Files: `backend/tests/test_services.py` (modify) or a new `backend/tests/test_credential_tester.py` (create)
|
||||
- Lines: ~40
|
||||
- Dependencies: 1.2
|
||||
- Details: Test the shared helper directly: (a) `requests.HTTPError` with a mock response `.status_code = 401` → auth message; (b) `requests.ConnectionError("getaddrinfo failed")` → "Host not found"; (c) `requests.Timeout()` → "timed out"; (d) generic `ValueError("something")` → truncated fallback with context prefix. Assert `ok=False` and the detail string contains expected keywords.
|
||||
|
||||
- [ ] **1.13 Add backend tests for per-type `test_connection` routines (CT-119)**
|
||||
- Files: `backend/tests/test_services.py` (modify) or `backend/tests/test_credential_tester.py` (create/modify)
|
||||
- Lines: ~100
|
||||
- Dependencies: 1.3–1.9
|
||||
- Details: For each of the 7 remote types, add at least two tests: (a) SUCCESS — mock the client/request to return valid data, assert `{ok: True, detail: ..., evidence: ...}`; (b) FAILURE — mock the client/request to raise, assert `{ok: False, detail: <readable message>, evidence: None}`. Specific cases:
|
||||
- **qbittorrent**: success returns version; `RuntimeError("qBittorrent login failed: Fails.")` → detail mentions "Authentication failed".
|
||||
- **prometheus**: success returns gateway-reachable evidence; HTTP 401 → auth message.
|
||||
- **alertmanager**: success returns version; connection refused → unreachable message.
|
||||
- **jellyfin**: success returns user count; HTTP 401 → auth message.
|
||||
- **authentik**: success returns user count; connection error → unreachable.
|
||||
- **ssh_tasks**: mock `build_ssh_client` to return a mock client; `.connect()` raises "authentication failed" → SSH auth message; `.connect()` succeeds → connected evidence.
|
||||
- **nextcloud**: success returns version; 404 → connection error.
|
||||
- Use `unittest.mock.patch` to mock at the right boundary (the client constructor or `requests.get/post`).
|
||||
|
||||
- [ ] **1.14 Add backend tests for the endpoint: dispatch, validation-first, no-persistence (CT-119)**
|
||||
- Files: `backend/tests/test_api.py` (modify) or `backend/tests/test_credential_tester.py` (create/modify)
|
||||
- Lines: ~50
|
||||
- Dependencies: 1.11
|
||||
- Details: (a) DISPATCH: `POST /api/services/test` with a `backups` body → `{ok: True, detail: "No connection test..."}` and no network call. (b) VALIDATION-FIRST: body with schema-less `base_url` for qbittorrent → HTTP 422 (not 200); assert the test_callable was NOT called (mock it, assert call count 0). (c) NO-PERSISTENCE: call `/test` with a valid qbittorrent body (test_callable mocked to return ok), assert `store.list_services()` count is unchanged before/after. (d) NO-SECRET-LOGS: use `caplog` at INFO level, call `/test` with `secrets: {"password": "hunter2"}`, assert no log line contains "hunter2".
|
||||
|
||||
- [ ] **1.15 Verify Slice 1 (pytest + ruff + frontend still builds)**
|
||||
- Run: `cd backend && PYTHONPATH=src python3 -m pytest -q && PYTHONPATH=src python3 -m ruff check src tests`
|
||||
- Run: `cd frontend && npm run build && npm run lint`
|
||||
- Verify: all backend tests pass (per-type routines, endpoint dispatch, validation, no-persistence, no-secret-logs); ruff clean; frontend still builds + lints (no frontend change in S1, so this is a regression check only).
|
||||
- **Risk flag:** the `test_callable` field on `ServiceDefinition` is a new field with a default. Verify that existing DEFINITION construction sites (all 8 integration modules) still compile — they should, since the default is `None`, but a frozen dataclass with field-ordering can surprise if any DEFINITION passes positional args. Check for positional-arg construction (unlikely — the codebase uses keyword args).
|
||||
|
||||
**Slice 1 total:** ~300–380 changed lines.
|
||||
|
||||
---
|
||||
|
||||
## Slice 2: Frontend Test UI — API client + hook + shared panel + wire into both surfaces + tests
|
||||
|
||||
**Goal:** Add a "Test credentials" button + result pill + Create/Save gating to BOTH the add-service dialog (`CreateServiceDialog` in `ServicesPage.tsx`) and the edit panel (`ServiceConfigEditor` in `Settings.tsx`). A shared `ServiceTestPanel` component avoids duplication.
|
||||
|
||||
**Satisfies:** CT-114, CT-115, CT-116, CT-117, CT-118, CT-120, CT-121.
|
||||
|
||||
- [ ] **2.1 Add `ServiceTestResult` type (CT-114)**
|
||||
- Files: `frontend/src/types/index.ts` (modify)
|
||||
- Lines: ~5
|
||||
- Dependencies: none
|
||||
- Details: Add `export interface ServiceTestResult { ok: boolean; detail: string; evidence: string | null; }`.
|
||||
|
||||
- [ ] **2.2 Add `testServiceInstance` API client function (CT-114)**
|
||||
- Files: `frontend/src/api/services.ts` (modify)
|
||||
- Lines: ~5
|
||||
- Dependencies: 2.1
|
||||
- Details: Add `export async function testServiceInstance(input: ServiceInstanceInput): Promise<ServiceTestResult> { return post<ServiceTestResult>("/api/services/test", input); }`. Import `ServiceTestResult` from `../types`. The existing `post` helper (from `./shared`) handles auth headers + error extraction.
|
||||
|
||||
- [ ] **2.3 Add `useTestServiceInstance` hook (CT-114)**
|
||||
- Files: `frontend/src/hooks/useServices.ts` (modify)
|
||||
- Lines: ~8
|
||||
- Dependencies: 2.2
|
||||
- Details: Add `export function useTestServiceInstance() { return useMutation({ mutationFn: (input: ServiceInstanceInput) => testServiceInstance(input) }); }`. Import `useMutation` from `@tanstack/react-query` (already imported in the file). Import `testServiceInstance` from `../api/services`. No cache invalidation needed — the test is a one-shot mutation.
|
||||
|
||||
- [ ] **2.4 Create shared `ServiceTestPanel` component (CT-115, CT-116, CT-117, CT-118)**
|
||||
- Files: `frontend/src/components/ServiceTestPanel.tsx` (NEW)
|
||||
- Lines: ~70
|
||||
- Dependencies: 2.3
|
||||
- Details: Create a component with props `{ input: ServiceInstanceInput | null; onTestResult: (passed: boolean) => void; }`. Internal state: `result: ServiceTestResult | null`, `saveAnyway: boolean`. `useEffect([input])` clears `result` and calls `onTestResult(false)` whenever `input` changes (new object reference on every field edit — CT-118). A "Test credentials" `Button` (variant outline, size sm) fires `testService.mutateAsync(input)`; while pending shows "Testing…" and is disabled. On result: render an `Alert` — green/default variant with `✓ Connected — {evidence}` on success, destructive variant with `✗ {detail}` on failure. A "Save anyway (skip test)" checkbox toggles `saveAnyway`; when checked, calls `onTestResult(true)` regardless of test outcome (CT-117). The parent reads the gating signal via the `onTestResult` callback. Import `useTestServiceInstance`, `Alert`/`AlertDescription` from `@/components/ui/alert`, `Button` from `@/components/ui/button`.
|
||||
|
||||
- [ ] **2.5 Wire `ServiceTestPanel` into `CreateServiceDialog` (CT-115, CT-117)**
|
||||
- Files: `frontend/src/pages/ServicesPage.tsx` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 2.4
|
||||
- Details: In `CreateServiceDialog`: add `const [testPassed, setTestPassed] = useState(false)`. Reset it in `reset()`. Build `testInput` from the current draft (null if no draft). Render `<ServiceTestPanel input={testInput} onTestResult={setTestPassed} />` below the config/secret fields and above the footer. Update `DialogFooter`'s `confirmDisabled` to include `!testPassed` (i.e. `confirmDisabled={!draft.name.trim() || saveService.isPending || !testPassed}`). Import `ServiceTestPanel` from `../components/ServiceTestPanel`.
|
||||
|
||||
- [ ] **2.6 Wire `ServiceTestPanel` into `ServiceConfigEditor` in `Settings.tsx` (CT-115, CT-117)**
|
||||
- Files: `frontend/src/pages/Settings.tsx` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 2.4
|
||||
- Details: In `ServiceConfigEditor` (NOT `ServicePage.tsx` — per design source finding §0, service editing lives in `Settings.tsx`): add `const [testPassed, setTestPassed] = useState(false)`. Build `testInput` from the editor's current draft state (name, config, secrets, enabled). Render `<ServiceTestPanel input={testInput} onTestResult={setTestPassed} />` below the form fields. Gate the Save button on `!testPassed` (disabled until test passes or Save anyway is checked). Reset `testPassed` when switching instances (if the editor has an instance-switch effect). Import `ServiceTestPanel`.
|
||||
- **Risk flag:** the spec CT-115 says "ServicePage.tsx" — that is textual drift. The actual edit surface is `Settings.tsx::ServiceConfigEditor`. This task targets the correct file.
|
||||
|
||||
- [ ] **2.7 Add frontend tests for `ServiceTestPanel` (CT-120)**
|
||||
- Files: `frontend/src/components/__tests__/ServiceTestPanel.test.tsx` (NEW)
|
||||
- Lines: ~80
|
||||
- Dependencies: 2.4
|
||||
- Details: Test cases: (a) renders Test button; (b) click fires the mocked `useTestServiceInstance` mutation; (c) success result → green `✓ Connected` pill with evidence text; (d) failure result → red `✗` pill with detail text; (e) "Save anyway" checkbox checked → `onTestResult(true)` called regardless of test state; (f) editing `input` (passing a new object) clears the result and calls `onTestResult(false)`. Mock `useTestServiceInstance` via `vi.mock("../../hooks/useServices", ...)`. Use `@testing-library/react` + `@testing-library/user-event`.
|
||||
|
||||
- [ ] **2.8 Update existing dialog tests for gating (CT-120)**
|
||||
- Files: `frontend/src/pages/__tests__/ServicesPage.test.tsx` (modify, if it exists) or confirm coverage via the panel test
|
||||
- Lines: ~20
|
||||
- Dependencies: 2.5
|
||||
- Details: If `ServicesPage.test.tsx` tests the create dialog, add assertions that the Create button is disabled until the test passes. Mock the test mutation to return `{ok: true}` and verify the button enables. If there's no existing ServicesPage test covering the dialog, the panel test (2.7) covers the gating behavior adequately — note the coverage decision.
|
||||
|
||||
- [ ] **2.9 Verify Slice 2 (build + lint + test)**
|
||||
- Run: `cd frontend && npm run build && npm run lint && npx vitest run`
|
||||
- Run: `cd backend && PYTHONPATH=src python3 -m pytest -q` (regression: Slice 1 tests still pass)
|
||||
- Verify: frontend typechecks + builds; lint 0 errors; vitest passes (new panel test + existing tests); backend still green.
|
||||
- **Risk flag:** `ServiceConfigEditor` in `Settings.tsx` is a large component (~160 lines). The wiring (state + panel render + button gating) must be surgical — do not refactor the editor. Add the `testPassed` state, the `testInput` build, the panel render, and the button `disabled` prop only.
|
||||
|
||||
**Slice 2 total:** ~250–320 changed lines.
|
||||
|
||||
---
|
||||
|
||||
## Integration verification (post-slice)
|
||||
|
||||
- [ ] **3.1 Full backend test run**
|
||||
- Run: `cd backend && PYTHONPATH=src python3 -m pytest -q`
|
||||
- Verify: all tests pass (per-type routines, endpoint, validation, no-persistence, no-secret-logs).
|
||||
|
||||
- [ ] **3.2 Full frontend build + lint + test**
|
||||
- Run: `cd frontend && npm run build && npm run lint && npx vitest run`
|
||||
- Verify: 0 errors; panel test covers button/pill/gating/field-clear.
|
||||
|
||||
- [ ] **3.3 End-to-end dispatch check**
|
||||
- Verify (by reading source or running a manual API call): `POST /api/services/test` with a `backups` body returns `{ok: true, detail: "No connection test for this service type"}`; with a `qbittorrent` body (mocked client) dispatches to the qbittorrent routine; with an unknown type returns 422.
|
||||
|
||||
- [ ] **3.4 No-persistence check**
|
||||
- Verify (by test in 1.14): calling `/test` does not create a service row.
|
||||
|
||||
- [ ] **3.5 No-secret-logs check**
|
||||
- Verify (by test in 1.14): no log line contains a secret value.
|
||||
|
||||
---
|
||||
|
||||
## Risk flags summary
|
||||
|
||||
1. **(a) CT-115 spec drift — edit surface is `Settings.tsx::ServiceConfigEditor`, NOT `ServicePage.tsx`.** Task 2.6 targets the correct file. The spec's literal text is stale; the design (§0) corrected it. Do not add a Test button to `ServicePage.tsx` (it's a read-only tabbed view with no edit form).
|
||||
|
||||
2. **(b) ssh_tasks test must NOT duplicate `test_machine_ssh`.** `test_machine_ssh` (settings.py:122–180) is a router endpoint, not a reusable function. Task 1.8 uses `build_ssh_client(store, ServiceRecord)` from `task_runner.py` + `.connect()`, then translates errors inline using the same message patterns. No import of `test_machine_ssh`.
|
||||
|
||||
3. **(c) The shared `ServiceTestPanel` must work identically in both surfaces.** Tasks 2.5 + 2.6 wire the same component into `CreateServiceDialog` and `ServiceConfigEditor`. The panel's `input` prop is an object built from the parent's draft state; `onTestResult` is a callback the parent uses for gating. The panel owns the Test button, result pill, and Save-anyway checkbox; the parent owns the confirm button's `disabled` prop.
|
||||
|
||||
4. **(d) `ServiceDefinition` is a frozen dataclass.** Adding `test_callable` with a default `None` as the LAST field works with dataclass field-ordering. Verify no DEFINITION is constructed with positional args (the codebase uses keyword args, so this should be safe). Task 1.15 checks this.
|
||||
|
||||
5. **(e) Circular import risk for `SettingsStore` in `TestCallable`.** The type alias references `SettingsStore` (from `services.settings_store`). Use a string forward-reference (`"SettingsStore"`) in the type alias to avoid importing `settings_store` into `integrations/base.py` at module level. The actual `SettingsStore` is passed at runtime by the endpoint handler. Task 1.1 handles this.
|
||||
|
||||
6. **(f) `Settings.tsx::ServiceConfigEditor` is a large component.** Task 2.6 wiring must be surgical (add state + input build + panel render + button disabled prop). Do NOT refactor the editor or touch unrelated fields.
|
||||
Reference in New Issue
Block a user