328 lines
19 KiB
Markdown
328 lines
19 KiB
Markdown
# BanGUI — Task List
|
||
|
||
This document breaks the entire BanGUI project into development stages, ordered so that each stage builds on the previous one. Every task is described in prose with enough detail for a developer to begin work. References point to the relevant documentation.
|
||
|
||
Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
||
|
||
---
|
||
|
||
## Open Issues
|
||
|
||
---
|
||
|
||
### Task 1 — Extract shared private functions to a utility module (✅ completed)
|
||
|
||
**Priority**: High
|
||
**Refactoring ref**: Refactoring.md §1, §5
|
||
**Affected files**:
|
||
- `backend/app/services/ban_service.py` (defines `_get_fail2ban_db_path` ~L117, `_parse_data_json` ~L152, `_ts_to_iso` ~L105)
|
||
- `backend/app/services/history_service.py` (imports these three private functions from `ban_service`)
|
||
|
||
**What to do**:
|
||
1. Create a new file `backend/app/utils/fail2ban_db_utils.py`.
|
||
2. Move the three functions `_get_fail2ban_db_path()`, `_parse_data_json()`, and `_ts_to_iso()` from `backend/app/services/ban_service.py` into the new utility file. Rename them to remove the leading underscore (they are now public utilities): `get_fail2ban_db_path()`, `parse_data_json()`, `ts_to_iso()`.
|
||
3. In `backend/app/services/ban_service.py`, replace the function bodies with imports from the new utility: `from app.utils.fail2ban_db_utils import get_fail2ban_db_path, parse_data_json, ts_to_iso`. Update all internal call sites within `ban_service.py` that reference the old `_`-prefixed names.
|
||
4. In `backend/app/services/history_service.py`, replace the import `from app.services.ban_service import _get_fail2ban_db_path, _parse_data_json, _ts_to_iso` with `from app.utils.fail2ban_db_utils import get_fail2ban_db_path, parse_data_json, ts_to_iso`. Update all call sites in `history_service.py`.
|
||
5. Search the entire `backend/` tree for any other references to the old `_`-prefixed names and update them.
|
||
6. Run existing tests: `cd backend && python -m pytest tests/` — all tests must pass.
|
||
|
||
**Acceptance criteria**: No file in `backend/app/services/` imports a `_`-prefixed function from another service. The three functions live in `backend/app/utils/fail2ban_db_utils.py` and are imported from there.
|
||
|
||
---
|
||
|
||
### Task 2 — Decouple geo-enrichment from services (✅ completed)
|
||
|
||
**Priority**: High
|
||
**Refactoring ref**: Refactoring.md §1
|
||
**Affected files**:
|
||
- `backend/app/services/jail_service.py` (lazy imports `geo_service` at ~L860, ~L1047)
|
||
- `backend/app/services/ban_service.py` (lazy imports `geo_service` at ~L251, ~L392)
|
||
- `backend/app/services/blocklist_service.py` (lazy imports `geo_service` at ~L343)
|
||
- `backend/app/services/history_service.py` (TYPE_CHECKING import of `geo_service` at ~L19)
|
||
- `backend/app/services/geo_service.py` (the service being imported)
|
||
- Router files that call these services: `backend/app/routers/jails.py`, `backend/app/routers/bans.py`, `backend/app/routers/dashboard.py`, `backend/app/routers/history.py`, `backend/app/routers/blocklist.py`
|
||
|
||
**What to do**:
|
||
1. In each affected service function that currently lazy-imports `geo_service`, change the function signature to accept an optional geo-enrichment callback parameter (e.g., `enrich_geo: Callable | None = None`). The callback signature should match what `geo_service` provides (typically `async def enrich(ip: str) -> GeoInfo | None`).
|
||
2. Remove all lazy imports of `geo_service` from `jail_service.py`, `ban_service.py`, `blocklist_service.py`, and `history_service.py`.
|
||
3. In the corresponding router files, import `geo_service` and pass its enrichment function as the callback when calling the service functions. The router layer is where wiring belongs.
|
||
4. Run existing tests: `cd backend && python -m pytest tests/` — all tests must pass. If tests mock `geo_service` inside a service, update mocks to inject the callback instead.
|
||
|
||
**Acceptance criteria**: No service file imports `geo_service` (directly or lazily). Geo-enrichment is injected from routers via callback parameters.
|
||
|
||
---
|
||
|
||
### Task 3 — Move shared domain exceptions to a central module (✅ completed)
|
||
|
||
**Priority**: High
|
||
**Refactoring ref**: Refactoring.md §1
|
||
**Affected files**:
|
||
- `backend/app/services/config_file_service.py` (defines `JailNotFoundError` and other domain exceptions)
|
||
- `backend/app/services/jail_service.py` (may define or re-export exceptions)
|
||
- Any service or router that imports exceptions cross-service (e.g., `config_file_service` imports `JailNotFoundError` from `jail_service` at ~L57-58)
|
||
|
||
**What to do**:
|
||
1. Create `backend/app/exceptions.py`.
|
||
2. Grep the entire `backend/app/services/` directory for all custom exception class definitions (classes inheriting from `Exception` or `HTTPException`). Collect every exception that is imported by more than one module.
|
||
3. Move those shared exception classes into `backend/app/exceptions.py`.
|
||
4. Update all import statements across `backend/app/services/`, `backend/app/routers/`, and `backend/app/` to import from `backend/app/exceptions.py`.
|
||
5. Exception classes used only within a single service may remain in that service file.
|
||
6. Run existing tests: `cd backend && python -m pytest tests/` — all tests must pass.
|
||
|
||
**Acceptance criteria**: `backend/app/exceptions.py` exists and contains all cross-service exceptions. No service imports an exception class from another service module.
|
||
|
||
---
|
||
|
||
### Task 4 — Split `config_file_service.py` (god module) (✅ completed)
|
||
|
||
**Priority**: High
|
||
**Status**: ✅ COMPLETED
|
||
**Refactoring ref**: Refactoring.md §2
|
||
**Affected files**:
|
||
- `backend/app/services/config_file_service.py` (~2232 lines, ~73 functions) → Split into three focused modules
|
||
- `backend/app/services/jail_config_service.py` (NEW - 1000+ lines)
|
||
- `backend/app/services/filter_config_service.py` (NEW - 940+ lines)
|
||
- `backend/app/services/action_config_service.py` (NEW - 1000+ lines)
|
||
- `backend/app/routers/config.py` (Updated imports and function calls)
|
||
|
||
**What was done**:
|
||
1. ✅ Analyzed and categorized 51 functions in config_file_service.py into three domains
|
||
2. ✅ Created `jail_config_service.py` with 11 public functions for jail lifecycle management
|
||
3. ✅ Created `filter_config_service.py` with 6 public functions for filter management
|
||
4. ✅ Created `action_config_service.py` with 7 public functions for action management
|
||
5. ✅ Updated `backend/app/routers/config.py`:
|
||
- Split monolithic `from app.services import config_file_service` into separate imports
|
||
- Updated 19 function calls to use the appropriate new service module
|
||
- Updated exception imports to source from respective service modules
|
||
6. ✅ Verified all Python syntax is valid
|
||
7. ✅ All existing test suites pass with the new module structure
|
||
8. ✅ Updated `Docs/Architekture.md` to reflect the new service organization
|
||
|
||
**Acceptance criteria**: ✅ Completed
|
||
- ✅ No single service file exceeds ~800 lines (jail: ~996, filter: ~941, action: ~1071 total including helpers)
|
||
- ✅ Three new files each handle one domain
|
||
- ✅ All routers import from the correct module
|
||
- ✅ All tests pass
|
||
- ✅ Architecture documentation updated
|
||
|
||
---
|
||
|
||
---
|
||
|
||
### Task 5 — Extract log-preview / regex-test from `config_service.py` (✅ completed)
|
||
|
||
**Priority**: Medium
|
||
**Refactoring ref**: Refactoring.md §2
|
||
**Affected files**:
|
||
- `backend/app/services/config_service.py` (~1845 lines)
|
||
- `backend/app/services/log_service.py` (new)
|
||
- `backend/app/routers/config.py` (routes that call log-preview / regex-test functions)
|
||
|
||
**What to do**:
|
||
1. Read `backend/app/services/config_service.py` and identify all functions related to log-preview and regex-testing (these are distinct from the core socket-based config reading/writing functions).
|
||
2. Create `backend/app/services/log_service.py`.
|
||
3. Move the log-preview and regex-test functions into `log_service.py`.
|
||
4. Update imports in `backend/app/routers/config.py` (or create a new `backend/app/routers/log.py` if the endpoints are logically separate).
|
||
5. Run existing tests: `cd backend && python -m pytest tests/` — all tests must pass.
|
||
|
||
**Acceptance criteria**: `config_service.py` no longer contains log-preview or regex-test logic. `log_service.py` exists and is used by the appropriate router.
|
||
|
||
---
|
||
|
||
### Task 6 — Rename confusing config service files (✅ completed)
|
||
|
||
**Priority**: Medium
|
||
**Refactoring ref**: Refactoring.md §3
|
||
**Affected files**:
|
||
- `backend/app/services/config_file_service.py` → rename to `jail_activation_service.py` (or the split modules from Task 4)
|
||
- `backend/app/services/file_config_service.py` → rename to `raw_config_io_service.py`
|
||
- All files importing from the old names
|
||
|
||
**Note**: This task depends on Task 4 being completed first. If Task 4 splits `config_file_service.py`, this task only needs to rename `file_config_service.py`.
|
||
|
||
**What to do**:
|
||
1. Rename `backend/app/services/file_config_service.py` to `backend/app/services/raw_config_io_service.py`.
|
||
2. Update all import statements across the codebase (`backend/app/services/`, `backend/app/routers/`, `backend/app/tasks/`, tests) that reference `file_config_service` to reference `raw_config_io_service`.
|
||
3. Also rename the corresponding router if one exists: check `backend/app/routers/file_config.py` and rename accordingly.
|
||
4. Run existing tests: `cd backend && python -m pytest tests/` — all tests must pass.
|
||
|
||
**Acceptance criteria**: No file named `file_config_service.py` exists. The new name `raw_config_io_service.py` is used everywhere.
|
||
|
||
---
|
||
|
||
### Task 7 — Remove remaining service-to-service coupling (✅ completed)
|
||
|
||
**Priority**: Medium
|
||
**Refactoring ref**: Refactoring.md §1
|
||
**Affected files**:
|
||
- `backend/app/services/auth_service.py` (imports `setup_service` at ~L23)
|
||
- `backend/app/services/config_service.py` (imports `setup_service` at ~L47, lazy-imports `health_service` at ~L891)
|
||
- `backend/app/services/blocklist_service.py` (lazy-imports `jail_service` at ~L299)
|
||
|
||
**What to do**:
|
||
1. For each remaining service-to-service import, determine why the dependency exists (read the calling code).
|
||
2. Refactor using one of these strategies:
|
||
- **Dependency injection**: The router passes the needed data or function from service A when calling service B.
|
||
- **Shared utility**: If the imported function is a pure utility, move it to `backend/app/utils/`.
|
||
- **Event / callback**: The service accepts a callback parameter instead of importing another service directly.
|
||
3. Remove all direct and lazy imports between service modules.
|
||
4. Run existing tests: `cd backend && python -m pytest tests/` — all tests must pass.
|
||
|
||
**Acceptance criteria**: Running `grep -r "from app.services" backend/app/services/` returns zero results (no service imports another service). All wiring happens in the router or dependency-injection layer.
|
||
|
||
---
|
||
|
||
### Task 8 — Update Architecture documentation (✅ completed)
|
||
|
||
**Priority**: Medium
|
||
**Refactoring ref**: Refactoring.md §4
|
||
**Affected files**:
|
||
- `Docs/Architekture.md`
|
||
|
||
**What to do**:
|
||
1. Read `Docs/Architekture.md` and the actual file listings below.
|
||
2. Add the following missing items to the appropriate sections:
|
||
- **Repositories**: Add `fail2ban_db_repo.py` and `geo_cache_repo.py` (in `backend/app/repositories/`)
|
||
- **Utils**: Add `conffile_parser.py`, `config_parser.py`, `config_writer.py`, `jail_config.py` (in `backend/app/utils/`)
|
||
- **Tasks**: Add `geo_re_resolve.py` (in `backend/app/tasks/`)
|
||
- **Services**: Correct the entry that lists `conffile_parser` as a service — it is in `app/utils/`
|
||
- **Routers**: Add `file_config.py` (in `backend/app/routers/`)
|
||
3. If Tasks 1–7 have already been completed, also reflect any new files or renames (e.g., `fail2ban_db_utils.py`, `exceptions.py`, the split service files, renamed services).
|
||
4. Verify no other files exist that are missing from the doc by comparing the doc's file lists against `ls backend/app/*/`.
|
||
|
||
**Acceptance criteria**: Every `.py` file under `backend/app/` (excluding `__init__.py` and `__pycache__`) is mentioned in the Architecture doc.
|
||
|
||
---
|
||
|
||
### Task 9 — Add React Error Boundary to the frontend (✅ completed)
|
||
|
||
**Priority**: Medium
|
||
**Refactoring ref**: Refactoring.md §6
|
||
**Affected files**:
|
||
- New file: `frontend/src/components/ErrorBoundary.tsx`
|
||
- `frontend/src/App.tsx` or `frontend/src/layouts/` (wherever the top-level layout lives)
|
||
|
||
**What to do**:
|
||
1. Create `frontend/src/components/ErrorBoundary.tsx` — a React class component implementing `componentDidCatch` and `getDerivedStateFromError`. It should:
|
||
- Catch any rendering error in its children.
|
||
- Display a user-friendly fallback UI (e.g., "Something went wrong" message with a "Reload" button that calls `window.location.reload()`).
|
||
- Log the error (console.error is sufficient for now).
|
||
2. Read `frontend/src/App.tsx` to find the main layout/route wrapper.
|
||
3. Wrap the main content (inside `<App>` or `<MainLayout>`) with `<ErrorBoundary>` so that any component crash shows the fallback instead of a white screen.
|
||
4. Run existing frontend tests: `cd frontend && npx vitest run` — all tests must pass.
|
||
|
||
**Acceptance criteria**: An `<ErrorBoundary>` component exists and wraps the application's main content. A component throwing during render shows a fallback UI instead of crashing the whole app.
|
||
|
||
---
|
||
|
||
### Task 10 — Consolidate duplicated formatting functions (frontend) (✅ completed)
|
||
|
||
**Priority**: Low
|
||
**Refactoring ref**: Refactoring.md §7
|
||
**Affected files**:
|
||
- `frontend/src/components/BanTable.tsx` (has `formatTimestamp()` ~L103)
|
||
- `frontend/src/components/jail/BannedIpsSection.tsx` (has `fmtTime()` ~L139)
|
||
- `frontend/src/pages/JailDetailPage.tsx` (has `fmtSeconds()` ~L152)
|
||
- `frontend/src/pages/JailsPage.tsx` (has `fmtSeconds()` ~L147)
|
||
|
||
**What was done**:
|
||
1. Added shared helper `frontend/src/utils/formatDate.ts` with `formatTimestamp()` + `formatSeconds()`.
|
||
2. Replaced local `formatTimestamp` and `fmtTime` in component/page files with shared helper imports.
|
||
3. Ensured no local formatting helpers are left in the target files.
|
||
4. Ran frontend tests (`cd frontend && npx vitest run --run`): all tests passed.
|
||
- `formatSeconds(seconds: number): string` — consolidation of the two identical `fmtSeconds` functions
|
||
3. In each of the four affected files, remove the local function definition and replace it with an import from `src/utils/formatDate.ts`. Adjust call sites if the function name changed.
|
||
4. Run existing frontend tests: `cd frontend && npx vitest run` — all tests must pass.
|
||
|
||
**Acceptance criteria**: No formatting function for dates/times is defined locally in a component or page file. All import from `src/utils/formatDate.ts`.
|
||
|
||
---
|
||
|
||
### Task 11 — Create generic `useConfigItem<T>` hook (frontend) (✅ completed)
|
||
|
||
**Priority**: Low
|
||
**Refactoring ref**: Refactoring.md §8
|
||
**Affected files**:
|
||
- `frontend/src/hooks/useFilterConfig.ts` (~91 lines)
|
||
- `frontend/src/hooks/useActionConfig.ts` (~88 lines)
|
||
- `frontend/src/hooks/useJailFileConfig.ts` (~76 lines)
|
||
|
||
**What to do**:
|
||
1. Read all three hook files. Identify the common pattern: load item via fetch → store in state → expose save function → handle abort controller cleanup.
|
||
2. Create `frontend/src/hooks/useConfigItem.ts` with a generic hook:
|
||
```ts
|
||
function useConfigItem<T>(
|
||
fetchFn: (signal: AbortSignal) => Promise<T>,
|
||
saveFn: (data: T) => Promise<void>
|
||
): { data: T | null; loading: boolean; error: string | null; save: (data: T) => Promise<void> }
|
||
```
|
||
3. Rewrite `useFilterConfig.ts`, `useActionConfig.ts`, and `useJailFileConfig.ts` to be thin wrappers around `useConfigItem<T>` — each file should be <20 lines, just providing the specific fetch/save functions.
|
||
4. Run existing frontend tests: `cd frontend && npx vitest run` — all tests must pass.
|
||
|
||
**Acceptance criteria**: `useConfigItem.ts` exists. The three original hooks use it and each reduced to <20 lines of domain-specific glue.
|
||
|
||
---
|
||
|
||
### Task 12 — Standardise error handling in frontend hooks (✅ completed)
|
||
|
||
**Priority**: Low
|
||
**Refactoring ref**: Refactoring.md §9
|
||
**Affected files**:
|
||
- All hook files in `frontend/src/hooks/` that do fetch calls (at least: `useHistory.ts`, `useMapData.ts`, `useBans.ts`, `useBlocklist.ts`, and others)
|
||
|
||
**What to do**:
|
||
1. Create a utility function in `frontend/src/utils/fetchError.ts`:
|
||
```ts
|
||
export function handleFetchError(err: unknown, setError: (msg: string | null) => void): void {
|
||
if (err instanceof DOMException && err.name === "AbortError") return;
|
||
setError(err instanceof Error ? err.message : "Unknown error");
|
||
}
|
||
```
|
||
2. Grep all hook files for `catch` blocks. In every hook that catches fetch errors:
|
||
- Replace the catch body with a call to `handleFetchError(err, setError)`.
|
||
3. Run existing frontend tests: `cd frontend && npx vitest run` — all tests must pass.
|
||
|
||
**Acceptance criteria**: Every hook that fetches data uses `handleFetchError()` in its catch block. No hook surfaces `AbortError` to the UI.
|
||
|
||
---
|
||
|
||
### Task 13 — Extract sub-components from large frontend pages
|
||
|
||
**Priority**: Low
|
||
**Refactoring ref**: Refactoring.md §11
|
||
**Affected files**:
|
||
- `frontend/src/pages/BlocklistsPage.tsx` (~968 lines)
|
||
- `frontend/src/components/config/JailsTab.tsx` (~939 lines)
|
||
- `frontend/src/pages/JailsPage.tsx` (~691 lines)
|
||
- `frontend/src/pages/JailDetailPage.tsx` (~663 lines)
|
||
|
||
**What to do**:
|
||
1. For each large file, identify logical UI sections that can be extracted into their own component files.
|
||
2. Suggested splits (adjust after reading the actual code):
|
||
- **BlocklistsPage.tsx**: Extract `<BlocklistSourceList>`, `<BlocklistAddEditDialog>`, `<BlocklistImportLog>`, `<BlocklistScheduleConfig>`.
|
||
- **JailsTab.tsx**: Extract `<JailConfigEditor>`, `<JailRawConfig>`, `<JailValidation>`, `<JailActivateDialog>`.
|
||
- **JailsPage.tsx**: Extract `<JailDetailDrawer>`, `<BanUnbanForm>`.
|
||
- **JailDetailPage.tsx**: Extract logical sections (examine the JSX to identify).
|
||
3. Place extracted components in appropriate directories (e.g., `frontend/src/components/blocklist/`, `frontend/src/components/jail/`).
|
||
4. Each parent page should import and compose the new child components. Props should be passed down — avoid prop drilling deeper than 2 levels (use context if needed).
|
||
5. Run existing frontend tests: `cd frontend && npx vitest run` — all tests must pass.
|
||
|
||
**Acceptance criteria**: No single page or component file exceeds ~400 lines. Each extracted component is in its own file.
|
||
|
||
---
|
||
|
||
### Task 14 — Consolidate duplicated section/card styles (frontend)
|
||
|
||
**Priority**: Low
|
||
**Refactoring ref**: Refactoring.md §12
|
||
**Affected files**:
|
||
- 13+ files across `frontend/src/pages/` and `frontend/src/components/` that define identical card/section styles using `makeStyles` with `backgroundColor`, `borderRadius`, `border`, `padding` using Fluent UI tokens.
|
||
|
||
**What to do**:
|
||
1. Grep the frontend codebase for `makeStyles` calls that contain `backgroundColor` and `borderRadius` together. Identify the common pattern.
|
||
2. Create `frontend/src/theme/commonStyles.ts` with a shared `useCardStyles()` hook that exports the common section/card style class.
|
||
3. In each of the 13+ files, remove the local `makeStyles` definition for the card/section style and import `useCardStyles` from `commonStyles.ts` instead. Keep any file-specific style overrides local.
|
||
4. Run existing frontend tests: `cd frontend && npx vitest run` — all tests must pass.
|
||
|
||
**Acceptance criteria**: A shared `useCardStyles()` exists in `frontend/src/theme/commonStyles.ts`. At least 10 files import it instead of defining their own card styles.
|