From 94ea0430e87557daec99851398b0574cba7fc02d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 11 Aug 2026 14:11:49 +0200 Subject: [PATCH 01/17] feat(dashboard,host): correlation id on errors, linkable module tiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two items from the screen-wireframe board. 1y — an error page told the user something broke but gave them nothing to quote in a support report. The correlation id is already on every log line for the request and echoed as X-Correlation-ID; render it too, so the page and the logs can be joined. Adds a shared CopyableId component, since retyping a uuid by hand is the failure mode it exists to prevent. 1g — dashboard module tiles were inert. Each now links to its module's own screen and shows that module's worst health status. Health checks gain a `module` attribution, stamped by the host around register_health_checks so module authors keep the existing add() signature. Link targets are gated client-side against the per-user `menus` prop rather than server-side: the stats payload is process-wide cached for 30s, so any per-user filtering there would leak across sessions. --- framework/core/simple_module_core/health.py | 16 +++++ framework/core/tests/test_health_registry.py | 35 ++++++++++ .../simple_module_hosting/_error_handlers.py | 13 +++- .../simple_module_hosting/app_builder.py | 5 ++ .../tests/test_error_page_shared_props.py | 11 ++++ host/client_app/pages/Error.tsx | 25 ++++++- host/locales/en.json | 4 +- host/locales/es.json | 4 +- modules/dashboard/dashboard/locales/en.json | 7 +- modules/dashboard/dashboard/locales/es.json | 7 +- modules/dashboard/dashboard/pages/Home.tsx | 43 +++++++++--- .../dashboard/pages/components/ModuleTile.tsx | 65 +++++++++++++++++++ modules/dashboard/dashboard/stats.py | 46 +++++++++++-- modules/dashboard/tests/test_dashboard.py | 21 ++++++ packages/i18n/src/generated-resources.ts | 5 ++ packages/i18n/src/keys.generated.ts | 7 ++ packages/ui/src/components/CopyableId.tsx | 64 ++++++++++++++++++ 17 files changed, 357 insertions(+), 21 deletions(-) create mode 100644 modules/dashboard/dashboard/pages/components/ModuleTile.tsx create mode 100644 packages/ui/src/components/CopyableId.tsx diff --git a/framework/core/simple_module_core/health.py b/framework/core/simple_module_core/health.py index 4c6f3f39..81f5ffb9 100644 --- a/framework/core/simple_module_core/health.py +++ b/framework/core/simple_module_core/health.py @@ -30,6 +30,9 @@ class HealthCheck: name: str check: HealthCheckFn + module: str = "" + """Module that contributed the check. Stamped by the registry during + ``register_health_checks``; module authors never set it by hand.""" class HealthRegistry: @@ -37,8 +40,21 @@ class HealthRegistry: def __init__(self) -> None: self._checks: list[HealthCheck] = [] + self._current_owner: str = "" + + def set_owner(self, module_name: str) -> None: + """Attribute subsequently-added checks to ``module_name``. + + The host calls this around each ``register_health_checks`` hook so a + check knows which module it belongs to without changing the ``add`` + signature module authors already use. Attribution is what lets the + dashboard show health per module rather than one global number. + """ + self._current_owner = module_name def add(self, check: HealthCheck) -> None: + if not check.module: + check.module = self._current_owner self._checks.append(check) @property diff --git a/framework/core/tests/test_health_registry.py b/framework/core/tests/test_health_registry.py index 1c0fb390..cbbb86b6 100644 --- a/framework/core/tests/test_health_registry.py +++ b/framework/core/tests/test_health_registry.py @@ -33,6 +33,41 @@ async def check_b() -> HealthCheckResult: reg.add(HealthCheck(name="b", check=check_b)) assert len(reg.all_checks) == 2 + async def test_checks_are_attributed_to_the_owning_module(self): + """The dashboard shows health per module, which needs this attribution.""" + reg = HealthRegistry() + + async def check() -> HealthCheckResult: + return HealthCheckResult(status=HealthStatus.HEALTHY) + + reg.set_owner("FileStorage") + reg.add(HealthCheck(name="s3", check=check)) + reg.set_owner("BackgroundTasks") + reg.add(HealthCheck(name="broker", check=check)) + + owners = {c.name: c.module for c in reg.all_checks} + assert owners == {"s3": "FileStorage", "broker": "BackgroundTasks"} + + async def test_explicit_module_survives_the_current_owner(self): + reg = HealthRegistry() + + async def check() -> HealthCheckResult: + return HealthCheckResult(status=HealthStatus.HEALTHY) + + reg.set_owner("Dashboard") + reg.add(HealthCheck(name="db", check=check, module="Users")) + assert reg.all_checks[0].module == "Users" + + async def test_unowned_checks_have_no_module(self): + """Checks added outside a register_health_checks hook belong to nobody.""" + reg = HealthRegistry() + + async def check() -> HealthCheckResult: + return HealthCheckResult(status=HealthStatus.HEALTHY) + + reg.add(HealthCheck(name="db", check=check)) + assert reg.all_checks[0].module == "" + async def test_check_result_defaults(self): result = HealthCheckResult(status=HealthStatus.HEALTHY) assert result.detail is None diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index e7c280b2..4737562b 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -34,7 +34,18 @@ async def render_error_page(request: Request, status_code: int, message: str) -> shared = getattr(request.state, "inertia_shared", None) if shared: inertia.share(**shared) - response = await inertia.render("Error", {"status": status_code, "message": message}) + # The correlation id is the only handle a user has on their own failed + # request — without it a support report is just "it broke". It is already + # on every log line for this request, so quoting it back makes the page + # and the logs joinable. + response = await inertia.render( + "Error", + { + "status": status_code, + "message": message, + "correlation_id": getattr(request.state, "correlation_id", "") or "", + }, + ) response.status_code = status_code return response except InertiaVersionConflictException as exc: diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index d712a5e0..70c96f4a 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -224,10 +224,15 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: mod.register_permissions(perm_registry) mod.register_feature_flags(ff_registry) _register_event_handlers(mod, event_bus, app) + health_registry.set_owner(mod.meta.name) mod.register_health_checks(health_registry) mod.register_public_routes(public_route_registry) mod.register_design_packs(design_pack_registry) + # Stop attributing to the last module in the loop — anything registered + # after this point belongs to no module in particular. + health_registry.set_owner("") + attach_public_routes(app, settings, public_route_registry) # Branding reads the packs off app.state directly: its API validates a diff --git a/framework/hosting/tests/test_error_page_shared_props.py b/framework/hosting/tests/test_error_page_shared_props.py index 68d0efa7..a2812867 100644 --- a/framework/hosting/tests/test_error_page_shared_props.py +++ b/framework/hosting/tests/test_error_page_shared_props.py @@ -61,6 +61,17 @@ async def test_error_page_keeps_its_own_props( props = _inertia_page((await authenticated_client.get(_MISSING_PATH)).text)["props"] assert props["status"] == _NOT_FOUND + async def test_error_page_carries_correlation_id( + self, authenticated_client: httpx.AsyncClient + ) -> None: + """The page shows this id so a support report can be joined to the logs.""" + resp = await authenticated_client.get(_MISSING_PATH) + props = _inertia_page(resp.text)["props"] + assert props.get("correlation_id"), f"no correlation_id on error page; props={sorted(props)}" + # Must be the same id the response header advertises, or quoting it + # back would point support at a different request. + assert props["correlation_id"] == resp.headers.get("x-correlation-id") + async def test_anonymous_error_page_still_renders(self, client: httpx.AsyncClient) -> None: """An unauthenticated 404 must not blow up on missing shared state.""" resp = await client.get("/health/definitely-not-real") diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index 4f40bcab..927e5b4d 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -1,5 +1,6 @@ import { Head, Link } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; +import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; import { ErrorScreen } from '@simple-module-py/ui/components/ErrorScreen'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Home, LifeBuoy } from 'lucide-react'; @@ -7,9 +8,10 @@ import { Home, LifeBuoy } from 'lucide-react'; interface Props { status: number; message: string; + correlation_id?: string; } -function ErrorPage({ status, message }: Props) { +function ErrorPage({ status, message, correlation_id }: Props) { const { t } = useT(); const titles: Record = { @@ -36,7 +38,26 @@ function ErrorPage({ status, message }: Props) { return ( <> - + + + {t(keys.host.error.correlation_id_label)} + + + + ) : undefined + } + > + ); +} From a2e1a2b57e257b23105a53eac86dfdac5e437d5e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 11 Aug 2026 14:18:41 +0200 Subject: [PATCH 02/17] feat(background_tasks,file_storage): ops status strip, file search + upload progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1t — the executions table had no status counts, so spotting a pile of failed jobs meant paging through the list. Adds a counts strip that doubles as a filter: clicking a tile filters to it, clicking again clears. Counts honour the search box but deliberately ignore the status filter, since the strip is how you pick a status. Failed/stuck go red only when non-zero — a permanent red zero trains people to ignore it. 1s — files could not be searched or filtered by type, and past 20 rows were simply unreachable (the page had no pager at all). Adds filename search, content-type filter with a `image/`-style family option, facet counts drawn from what is actually in the bucket, and the missing pagination controls. Filter clauses are shared between the page query and its count so a filter can never narrow the rows without narrowing the total. Uploads now report per-file byte progress as rows above the table, via XMLHttpRequest — fetch exposes no upload progress events in any current browser. Failed rows persist until dismissed rather than vanishing. --- .../background_tasks/endpoints/views.py | 2 + .../background_tasks/pages/Index.tsx | 9 ++ .../pages/components/StatusStrip.tsx | 66 +++++++++ .../background_tasks/service.py | 20 +++ .../background_tasks/tests/test_bg_service.py | 40 ++++++ .../file_storage/endpoints/views.py | 14 +- .../file_storage/file_storage/locales/en.json | 21 ++- .../file_storage/pages/Browse.tsx | 88 +++++++++++- .../pages/components/FileFilterBar.tsx | 93 +++++++++++++ .../pages/components/UploadDropzone.tsx | 35 ++--- .../pages/components/UploadProgressRows.tsx | 62 +++++++++ .../file_storage/pages/upload-queue.ts | 107 +++++++++++++++ modules/file_storage/file_storage/service.py | 54 +++++++- .../tests/test_file_storage_filters.py | 128 ++++++++++++++++++ packages/i18n/src/generated-resources.ts | 13 ++ packages/i18n/src/keys.generated.ts | 18 +++ 16 files changed, 737 insertions(+), 33 deletions(-) create mode 100644 modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx create mode 100644 modules/file_storage/file_storage/pages/components/FileFilterBar.tsx create mode 100644 modules/file_storage/file_storage/pages/components/UploadProgressRows.tsx create mode 100644 modules/file_storage/file_storage/pages/upload-queue.ts create mode 100644 modules/file_storage/tests/test_file_storage_filters.py diff --git a/modules/background_tasks/background_tasks/endpoints/views.py b/modules/background_tasks/background_tasks/endpoints/views.py index 6e7e4890..b0792759 100644 --- a/modules/background_tasks/background_tasks/endpoints/views.py +++ b/modules/background_tasks/background_tasks/endpoints/views.py @@ -37,10 +37,12 @@ async def index( page=page, per_page=PER_PAGE, ) + counts = await service.status_counts(task_name=task_name or None) return await inertia.render( "BackgroundTasks/Index", { "executions": [i.model_dump(mode="json") for i in response.items], + "status_counts": {s.value: counts.get(s.value, 0) for s in TaskStatus}, "pagination": { "page": response.page, "per_page": response.per_page, diff --git a/modules/background_tasks/background_tasks/pages/Index.tsx b/modules/background_tasks/background_tasks/pages/Index.tsx index 651cb837..6c54ab0f 100644 --- a/modules/background_tasks/background_tasks/pages/Index.tsx +++ b/modules/background_tasks/background_tasks/pages/Index.tsx @@ -23,6 +23,7 @@ import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedL import { Activity, Search, ServerCog } from 'lucide-react'; import { useEffect, useState } from 'react'; import { ExecutionRow, statusLabel } from './components/ExecutionRow'; +import { type StatusCounts, StatusStrip } from './components/StatusStrip'; import { STATUS_ORDER, VIEW_BASE } from './constants'; import { type Execution, retryExecution } from './retry'; @@ -36,6 +37,7 @@ interface Props { executions: Execution[]; pagination: Pagination; filters: { status: string; task_name: string }; + status_counts: StatusCounts; } const STATUS_ALL = '__all__'; @@ -53,6 +55,7 @@ function Index() { executions, pagination, filters: initialFilters, + status_counts: statusCounts, } = usePage<{ props: Props }>().props as unknown as Props; const { can } = usePermissions(); @@ -85,6 +88,12 @@ function Index() { title="Background Tasks" description="Monitor task executions and retry failed or stuck jobs." > + pushFilters({ status, task_name: search }, 1)} + /> +
diff --git a/modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx b/modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx new file mode 100644 index 00000000..38bf40cd --- /dev/null +++ b/modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx @@ -0,0 +1,66 @@ +import { type TaskStatus, TASK_STATUS } from '../constants'; +import { statusLabel } from './ExecutionRow'; + +export type StatusCounts = Partial>; + +interface Props { + counts: StatusCounts; + /** Currently filtered status, or '' for all. */ + active: string; + onSelect: (status: string) => void; +} + +/** + * Statuses worth a dedicated tile, in the order an operator triages them. + * Failed and stuck lead because they are the reason anyone opens this page. + */ +const TILES: TaskStatus[] = [ + TASK_STATUS.FAILED, + TASK_STATUS.STUCK, + TASK_STATUS.RETRYING, + TASK_STATUS.RUNNING, + TASK_STATUS.PENDING, + TASK_STATUS.SUCCESS, +]; + +/** Tiles that mean "someone needs to look at this" get an alarm colour, but + * only when they are non-zero — a permanently red zero trains people to + * ignore it. */ +const ALARMING = new Set([TASK_STATUS.FAILED, TASK_STATUS.STUCK]); + +export function StatusStrip({ counts, active, onSelect }: Props) { + return ( +
+ {TILES.map((status) => { + const count = counts[status] ?? 0; + const isActive = active === status; + const alarm = ALARMING.has(status) && count > 0; + return ( + + ); + })} +
+ ); +} diff --git a/modules/background_tasks/background_tasks/service.py b/modules/background_tasks/background_tasks/service.py index b3711cd0..c5693194 100644 --- a/modules/background_tasks/background_tasks/service.py +++ b/modules/background_tasks/background_tasks/service.py @@ -82,6 +82,26 @@ async def list( task_name=task_name, ) + async def status_counts(self, *, task_name: str | None = None) -> dict[str, int]: + """Count executions per status for the ops strip above the table. + + Deliberately ignores the status filter — the strip is how the operator + picks a status, so it has to keep showing the ones they aren't looking + at. It does honour ``task_name`` so the counts describe the same + result set the table is paging through. + + Statuses with no rows are omitted; callers fill in zeros. + """ + query = select(TaskExecution.status, func.count().label("n")) + if task_name: + query = query.where(TaskExecution.task_name.ilike(f"%{task_name}%")) + query = query.group_by(TaskExecution.status) + + rows = (await self.db.execute(query)).all() + # `status` is a TaskStatus (StrEnum) on Postgres but comes back as a + # plain str on SQLite; normalise so the JSON keys match either way. + return {str(getattr(row[0], "value", row[0])): int(row[1]) for row in rows} + async def get(self, execution_id: uuid.UUID) -> TaskExecutionDetail | None: row = await self.db.get(TaskExecution, execution_id) if row is None: diff --git a/modules/background_tasks/tests/test_bg_service.py b/modules/background_tasks/tests/test_bg_service.py index 8614cdcf..3a33a404 100644 --- a/modules/background_tasks/tests/test_bg_service.py +++ b/modules/background_tasks/tests/test_bg_service.py @@ -99,6 +99,46 @@ async def test_filters_by_task_name_substring( assert [i.task_name for i in resp.items] == ["orders.send_receipt"] +class TestStatusCounts: + """Feeds the failed/stuck ops strip above the executions table.""" + + async def test_counts_are_grouped_by_status( + self, db_session: AsyncSession, service: BackgroundTaskService + ): + for status in (TaskStatus.FAILED, TaskStatus.FAILED, TaskStatus.SUCCESS): + db_session.add(_make_row(status=status)) + await db_session.flush() + + counts = await service.status_counts() + assert counts[TaskStatus.FAILED.value] == 2 + assert counts[TaskStatus.SUCCESS.value] == 1 + + async def test_empty_table_yields_no_counts(self, service: BackgroundTaskService): + assert await service.status_counts() == {} + + async def test_search_narrows_the_counts( + self, db_session: AsyncSession, service: BackgroundTaskService + ): + """The strip must describe the same rows the table is paging through.""" + db_session.add(_make_row(task_name="orders.send_receipt", status=TaskStatus.FAILED)) + db_session.add(_make_row(task_name="users.sync", status=TaskStatus.FAILED)) + await db_session.flush() + + counts = await service.status_counts(task_name="receipt") + assert counts == {TaskStatus.FAILED.value: 1} + + async def test_counts_keys_are_plain_strings( + self, db_session: AsyncSession, service: BackgroundTaskService + ): + """Postgres hands back the enum, SQLite a str — the page needs one shape.""" + db_session.add(_make_row(status=TaskStatus.STUCK)) + await db_session.flush() + + counts = await service.status_counts() + assert all(type(key) is str for key in counts) + assert "stuck" in counts + + class TestGet: async def test_returns_none_for_missing_id(self, service: BackgroundTaskService): assert await service.get(uuid.uuid4()) is None diff --git a/modules/file_storage/file_storage/endpoints/views.py b/modules/file_storage/file_storage/endpoints/views.py index 5098cb28..328c0919 100644 --- a/modules/file_storage/file_storage/endpoints/views.py +++ b/modules/file_storage/file_storage/endpoints/views.py @@ -24,9 +24,19 @@ async def browse( inertia: InertiaDep, page: int = 1, + q: str = "", + content_type: str = "", service: FileStorageService = Depends(get_file_storage_service), ) -> InertiaResponse: - items, total = await service.list_files(page=page, per_page=_PER_PAGE) + items, total = await service.list_files( + page=page, + per_page=_PER_PAGE, + search=q or None, + content_type=content_type or None, + ) + # Facets ignore the active filters so the dropdown keeps offering the + # other types — a filter that hides its own alternatives is a dead end. + facets = await service.content_type_facets() # The page name is hard-coded as a literal here (rather than via # ``constants.PAGE_BROWSE``) so the SM003/SM004 diagnostics — which do # static AST analysis and cannot resolve attribute access — pair this @@ -37,5 +47,7 @@ async def browse( { "files": [item.model_dump(mode="json") for item in items], "pagination": {"page": page, "perPage": _PER_PAGE, "total": total}, + "filters": {"q": q, "content_type": content_type}, + "content_types": facets, }, ) diff --git a/modules/file_storage/file_storage/locales/en.json b/modules/file_storage/file_storage/locales/en.json index 8814ff8e..9dd43b6f 100644 --- a/modules/file_storage/file_storage/locales/en.json +++ b/modules/file_storage/file_storage/locales/en.json @@ -7,7 +7,12 @@ "empty_title": "No files yet", "empty_description": "Upload your first file to get started.", "count_one": "{count} file", - "count_other": "{count} files" + "count_other": "{count} files", + "no_match_title": "No matching files", + "no_match_description": "No files match the current search or type filter.", + "previous": "Previous", + "next": "Next", + "page_of": "Page {page} of {total}" }, "table": { "filename": "Name", @@ -31,12 +36,24 @@ "uploaded": "\"{name}\" uploaded", "upload_failed": "Upload failed", "deleted": "\"{name}\" deleted", - "delete_failed": "Failed to delete file" + "delete_failed": "Failed to delete file", + "uploaded_count_one": "{count} file uploaded", + "uploaded_count_other": "{count} files uploaded", + "upload_failed_named": "\"{name}\" failed to upload" }, "errors": { "not_found": "File not found", "too_large": "File exceeds the maximum allowed size", "bad_type": "This file type is not allowed", "backend_error": "Storage backend error" + }, + "filters": { + "search_placeholder": "Search files by name…", + "type_label": "Type", + "type_all": "All types" + }, + "upload": { + "in_progress": "Uploading {name}", + "dismiss": "Dismiss" } } diff --git a/modules/file_storage/file_storage/pages/Browse.tsx b/modules/file_storage/file_storage/pages/Browse.tsx index 5619c5e0..79f96ef3 100644 --- a/modules/file_storage/file_storage/pages/Browse.tsx +++ b/modules/file_storage/file_storage/pages/Browse.tsx @@ -32,8 +32,13 @@ import { usePermissions } from '@simple-module-py/ui/hooks/use-permissions'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; import { Download, FileBox, Trash2 } from 'lucide-react'; import { toast } from 'sonner'; +import { type ContentTypeFacet, FileFilterBar } from './components/FileFilterBar'; import { UploadDropzone } from './components/UploadDropzone'; +import { UploadProgressRows } from './components/UploadProgressRows'; import { PERMISSIONS, ROUTES, UNKNOWN_UPLOADER } from './constants'; +import { useUploadQueue } from './upload-queue'; + +const COLUMN_COUNT = 5; interface StoredFile { id: string; @@ -53,6 +58,8 @@ interface Pagination { interface Props { files: StoredFile[]; pagination: Pagination; + filters: { q: string; content_type: string }; + content_types: ContentTypeFacet[]; } function formatBytes(n: number): string { @@ -64,18 +71,37 @@ function formatBytes(n: number): string { function Browse() { const page = usePage<{ props: Props }>(); - const { files, pagination } = page.props as unknown as Props; + const { files, pagination, filters, content_types: contentTypes } = page.props as unknown as Props; const { t } = useT(); const { can } = usePermissions(); const canUpload = can(PERMISSIONS.UPLOAD); const canDelete = can(PERMISSIONS.DELETE); + const { jobs, start, dismiss, busy } = useUploadQueue(); + + const isFiltered = !!(filters?.q || filters?.content_type); + + function navigate(next: { q: string; content_type: string }, page = 1) { + const params: Record = {}; + if (next.q) params.q = next.q; + if (next.content_type) params.content_type = next.content_type; + if (page > 1) params.page = String(page); + router.get(ROUTES.VIEW_BROWSE, params, { preserveState: true, preserveScroll: true }); + } + + // Changing a filter always returns to page 1 — page 4 of the previous + // filter rarely exists under the new one, and an empty page reads as + // "no results". + const applyFilters = (next: { q: string; content_type: string }) => navigate(next, 1); + + const currentFilters = { q: filters?.q ?? '', content_type: filters?.content_type ?? '' }; + const totalPages = Math.max(1, Math.ceil(pagination.total / pagination.perPage)); function handleDelete(file: StoredFile) { fetch(ROUTES.apiFile(file.id), { method: 'DELETE' }) .then((resp) => { if (!resp.ok) throw new Error('delete failed'); toast.success(t(keys.file_storage.toasts.deleted, { name: file.filename })); - router.reload({ only: ['files', 'pagination'] }); + router.reload({ only: ['files', 'pagination', 'content_types'] }); }) .catch(() => toast.error(t(keys.file_storage.toasts.delete_failed))); } @@ -86,8 +112,15 @@ function Browse() { : undefined} + actions={canUpload ? : undefined} > + + @@ -106,6 +139,11 @@ function Browse() { + {files.map((file) => ( {file.filename} @@ -166,16 +204,25 @@ function Browse() { ))} - {files.length === 0 && pagination.total === 0 && ( + {files.length === 0 && jobs.length === 0 && ( - + - {t(keys.file_storage.browse.empty_title)} + {/* "Nothing uploaded yet" is wrong — and discouraging — + when the bucket is full and the filter is just too + narrow. */} + + {isFiltered + ? t(keys.file_storage.browse.no_match_title) + : t(keys.file_storage.browse.empty_title)} + - {t(keys.file_storage.browse.empty_description)} + {isFiltered + ? t(keys.file_storage.browse.no_match_description) + : t(keys.file_storage.browse.empty_description)} @@ -184,6 +231,33 @@ function Browse() {
+ + {totalPages > 1 && ( +
+ + + {t(keys.file_storage.browse.page_of, { + page: pagination.page, + total: totalPages, + })} + + +
+ )} ); diff --git a/modules/file_storage/file_storage/pages/components/FileFilterBar.tsx b/modules/file_storage/file_storage/pages/components/FileFilterBar.tsx new file mode 100644 index 00000000..1bf235ee --- /dev/null +++ b/modules/file_storage/file_storage/pages/components/FileFilterBar.tsx @@ -0,0 +1,93 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Input } from '@simple-module-py/ui/components/ui/input'; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from '@simple-module-py/ui/components/ui/select'; +import { Search } from 'lucide-react'; +import { useEffect, useState } from 'react'; + +export interface ContentTypeFacet { + value: string; + count: number; +} + +interface Props { + search: string; + contentType: string; + facets: ContentTypeFacet[]; + onChange: (next: { q: string; content_type: string }) => void; +} + +/** Sentinel for "no type filter" — Radix Select forbids an empty item value. */ +export const TYPE_ALL = '__all__'; + +/** Group a bucketful of `image/png`, `image/jpeg`, … under one `image/` entry. */ +function families(facets: ContentTypeFacet[]): ContentTypeFacet[] { + const totals = new Map(); + for (const facet of facets) { + const family = `${facet.value.split('/')[0]}/`; + totals.set(family, (totals.get(family) ?? 0) + facet.count); + } + // A family with a single member says nothing the exact type doesn't. + return [...totals.entries()] + .filter(([family]) => facets.filter((f) => f.value.startsWith(family)).length > 1) + .map(([value, count]) => ({ value, count })); +} + +export function FileFilterBar({ search, contentType, facets, onChange }: Props) { + const { t } = useT(); + const [draft, setDraft] = useState(search); + + // Debounced so each keystroke isn't a round trip; the server value winning + // on change keeps Back/Forward navigation in sync with the box. + useEffect(() => setDraft(search), [search]); + useEffect(() => { + if (draft === search) return; + const timeout = setTimeout(() => onChange({ q: draft, content_type: contentType }), 300); + return () => clearTimeout(timeout); + }, [draft, search, contentType, onChange]); + + const grouped = families(facets); + + return ( +
+
+ + setDraft(e.target.value)} + placeholder={t(keys.file_storage.filters.search_placeholder)} + aria-label={t(keys.file_storage.filters.search_placeholder)} + /> +
+ +
+ ); +} diff --git a/modules/file_storage/file_storage/pages/components/UploadDropzone.tsx b/modules/file_storage/file_storage/pages/components/UploadDropzone.tsx index 5de3e75a..773287c9 100644 --- a/modules/file_storage/file_storage/pages/components/UploadDropzone.tsx +++ b/modules/file_storage/file_storage/pages/components/UploadDropzone.tsx @@ -1,37 +1,32 @@ -import { router } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Upload } from 'lucide-react'; -import { useRef, useState } from 'react'; +import { useRef } from 'react'; import { toast } from 'sonner'; -import { ROUTES } from '../constants'; +interface Props { + /** Hands the files to the page's upload queue, which reports progress. */ + onFiles: (files: FileList) => Promise<{ uploaded: number; failed: string[] }>; + busy: boolean; +} -export function UploadDropzone() { +export function UploadDropzone({ onFiles, busy }: Props) { const { t } = useT(); const inputRef = useRef(null); - const [busy, setBusy] = useState(false); async function handleFiles(files: FileList | null) { if (!files || files.length === 0) return; - setBusy(true); try { - for (const file of Array.from(files)) { - const form = new FormData(); - form.append('file', file); - const resp = await fetch(ROUTES.API_UPLOAD, { - method: 'POST', - body: form, - }); - if (!resp.ok) { - toast.error(t(keys.file_storage.toasts.upload_failed)); - } else { - toast.success(t(keys.file_storage.toasts.uploaded, { name: file.name })); - } + const { uploaded, failed } = await onFiles(files); + if (uploaded > 0) { + toast.success(t(keys.file_storage.toasts.uploaded_count, { count: uploaded })); + } + // Failures also leave a row on screen; the toast is for the case where + // the user has already scrolled away from the table. + for (const name of failed) { + toast.error(t(keys.file_storage.toasts.upload_failed_named, { name })); } - router.reload({ only: ['files', 'pagination'] }); } finally { - setBusy(false); if (inputRef.current) inputRef.current.value = ''; } } diff --git a/modules/file_storage/file_storage/pages/components/UploadProgressRows.tsx b/modules/file_storage/file_storage/pages/components/UploadProgressRows.tsx new file mode 100644 index 00000000..068b4aaa --- /dev/null +++ b/modules/file_storage/file_storage/pages/components/UploadProgressRows.tsx @@ -0,0 +1,62 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Button } from '@simple-module-py/ui/components/ui/button'; +import { Progress } from '@simple-module-py/ui/components/ui/progress'; +import { TableCell, TableRow } from '@simple-module-py/ui/components/ui/table'; +import { AlertCircle, X } from 'lucide-react'; + +import type { UploadJob } from '../upload-queue'; + +interface Props { + jobs: UploadJob[]; + onDismiss: (id: string) => void; + columnCount: number; +} + +/** In-flight and failed uploads, shown as rows above the stored files. */ +export function UploadProgressRows({ jobs, onDismiss, columnCount }: Props) { + const { t } = useT(); + if (jobs.length === 0) return null; + + return ( + <> + {jobs.map((job) => ( + + +
+ {job.status === 'error' ? ( +
+ + + ))} + + ); +} diff --git a/modules/file_storage/file_storage/pages/upload-queue.ts b/modules/file_storage/file_storage/pages/upload-queue.ts new file mode 100644 index 00000000..edbc766f --- /dev/null +++ b/modules/file_storage/file_storage/pages/upload-queue.ts @@ -0,0 +1,107 @@ +import { router } from '@inertiajs/react'; +import { useCallback, useState } from 'react'; + +import { ROUTES } from './constants'; + +export interface UploadJob { + id: string; + name: string; + size: number; + /** 0–100. Stays at 100 while the server finishes writing to the backend. */ + percent: number; + status: 'uploading' | 'done' | 'error'; +} + +let counter = 0; +function nextId(): string { + counter += 1; + return `upload-${counter}`; +} + +/** + * Uploads files one at a time, reporting byte progress per file. + * + * Uses XMLHttpRequest rather than fetch deliberately: fetch exposes no upload + * progress events in any current browser, and a large file uploading behind a + * spinner with no feedback is the exact complaint this replaces. + */ +export function useUploadQueue() { + const [jobs, setJobs] = useState([]); + + const patch = useCallback((id: string, changes: Partial) => { + setJobs((current) => current.map((j) => (j.id === id ? { ...j, ...changes } : j))); + }, []); + + const upload = useCallback( + (job: UploadJob, file: File) => + new Promise((resolve) => { + const form = new FormData(); + form.append('file', file); + + const xhr = new XMLHttpRequest(); + xhr.open('POST', ROUTES.API_UPLOAD); + xhr.upload.addEventListener('progress', (event) => { + if (!event.lengthComputable) return; + patch(job.id, { percent: Math.round((event.loaded / event.total) * 100) }); + }); + xhr.addEventListener('load', () => { + const ok = xhr.status >= 200 && xhr.status < 300; + patch(job.id, { percent: 100, status: ok ? 'done' : 'error' }); + resolve(ok); + }); + // A dropped connection and a rejected upload look the same to the + // user, so both land on the same visible error row. + xhr.addEventListener('error', () => { + patch(job.id, { status: 'error' }); + resolve(false); + }); + xhr.addEventListener('abort', () => { + patch(job.id, { status: 'error' }); + resolve(false); + }); + xhr.send(form); + }), + [patch], + ); + + const start = useCallback( + async (files: FileList | File[]): Promise<{ uploaded: number; failed: string[] }> => { + const list = Array.from(files); + if (list.length === 0) return { uploaded: 0, failed: [] }; + + const queued: UploadJob[] = list.map((file) => ({ + id: nextId(), + name: file.name, + size: file.size, + percent: 0, + status: 'uploading', + })); + setJobs((current) => [...current, ...queued]); + + let uploaded = 0; + const failed: string[] = []; + for (const [index, file] of list.entries()) { + const ok = await upload(queued[index], file); + if (ok) uploaded += 1; + else failed.push(file.name); + } + + if (uploaded > 0) router.reload({ only: ['files', 'pagination', 'content_types'] }); + // Completed rows disappear once the reloaded table can show the real + // record; failures stay until dismissed so they can't go unnoticed. + setJobs((current) => current.filter((j) => j.status === 'error')); + return { uploaded, failed }; + }, + [upload], + ); + + const dismiss = useCallback((id: string) => { + setJobs((current) => current.filter((j) => j.id !== id)); + }, []); + + // Derived, not tracked separately: a ref would not re-render the button and + // a second state field could disagree with the rows on screen. + const busy = jobs.some((j) => j.status === 'uploading'); + + return { jobs, start, dismiss, busy }; +} diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index 490a7ec9..3165ef49 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -146,12 +146,16 @@ async def list_files( page: int = 1, per_page: int = 20, created_by: str | None = None, + search: str | None = None, + content_type: str | None = None, ) -> tuple[list[StoredFileOut], int]: base = select(StoredFile) count_q = select(func.count()).select_from(StoredFile) - if created_by is not None: - base = base.where(StoredFile.created_by == created_by) - count_q = count_q.where(StoredFile.created_by == created_by) + for clause in self._filter_clauses( + created_by=created_by, search=search, content_type=content_type + ): + base = base.where(clause) + count_q = count_q.where(clause) total = (await self.db.execute(count_q)).scalar() or 0 result = await self.db.execute( @@ -163,6 +167,50 @@ async def list_files( items = [StoredFileOut.model_validate(_to_out_dict(r)) for r in rows] return items, total + @staticmethod + def _filter_clauses( + *, + created_by: str | None, + search: str | None, + content_type: str | None, + ) -> list: + """Build the WHERE clauses shared by the page query and its count. + + Kept in one place so a filter can never narrow the rows without also + narrowing the total — the bug that shows up as a pager offering page 3 + of an empty search. + """ + clauses = [] + if created_by is not None: + clauses.append(StoredFile.created_by == created_by) + if search: + clauses.append(StoredFile.filename.ilike(f"%{search}%")) + if content_type: + # A trailing "/" means a whole family ("image/"), anything else is + # an exact type ("application/pdf"). Families are what make the + # filter usable when a bucket holds nine kinds of image. + if content_type.endswith("/"): + clauses.append(StoredFile.content_type.ilike(f"{content_type}%")) + else: + clauses.append(StoredFile.content_type == content_type) + return clauses + + async def content_type_facets(self, *, created_by: str | None = None) -> list[dict]: + """Distinct content types present, with counts, for the filter dropdown. + + Offering the full IANA list would be noise; the only types worth + showing are the ones actually in the bucket. + """ + query = select(StoredFile.content_type, func.count().label("n")) + for clause in self._filter_clauses( + created_by=created_by, search=None, content_type=None + ): + query = query.where(clause) + query = query.group_by(StoredFile.content_type).order_by(StoredFile.content_type) + + rows = (await self.db.execute(query)).all() + return [{"value": str(row[0]), "count": int(row[1])} for row in rows] + async def get(self, file_id: uuid.UUID) -> StoredFile: row = await self.db.get(StoredFile, file_id) if row is None: diff --git a/modules/file_storage/tests/test_file_storage_filters.py b/modules/file_storage/tests/test_file_storage_filters.py new file mode 100644 index 00000000..d9952ce8 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_filters.py @@ -0,0 +1,128 @@ +"""Search and content-type filtering for the file browse screen. + +The screen shipped with neither, so a bucket past its first page was only +navigable by luck. These cover the filters and — importantly — that the +total travels with them, since a count that ignores the filter produces a +pager offering pages that render empty. +""" + +from __future__ import annotations + +from io import BytesIO + +from fastapi import UploadFile +from file_storage import constants +from file_storage.backends.filesystem import FilesystemBackend +from file_storage.service import FileStorageService +from file_storage.settings import FileStorageSettings +from sqlalchemy.ext.asyncio import AsyncSession + + +def _upload(name: str, content_type: str) -> UploadFile: + return UploadFile( + filename=name, + file=BytesIO(b"payload"), + headers={"content-type": content_type}, # type: ignore[arg-type] + ) + + +def _service(tmp_path, db_session: AsyncSession) -> FileStorageService: + settings = FileStorageSettings( + backend=constants.BackendId.FILESYSTEM, + fs_root_path=str(tmp_path), + ) + return FileStorageService(db_session, FilesystemBackend(root=tmp_path), settings) + + +async def _seed(svc: FileStorageService) -> None: + await svc.upload(_upload("q3-report.pdf", "application/pdf")) + await svc.upload(_upload("export-2026-08.csv", "text/csv")) + await svc.upload(_upload("logo.png", "image/png")) + await svc.upload(_upload("banner.jpeg", "image/jpeg")) + + +async def test_search_matches_filename_substring(tmp_path, db_session: AsyncSession): + svc = _service(tmp_path, db_session) + await _seed(svc) + + items, total = await svc.list_files(search="report") + assert [i.filename for i in items] == ["q3-report.pdf"] + assert total == 1 + + +async def test_search_is_case_insensitive(tmp_path, db_session: AsyncSession): + svc = _service(tmp_path, db_session) + await _seed(svc) + + items, _ = await svc.list_files(search="REPORT") + assert [i.filename for i in items] == ["q3-report.pdf"] + + +async def test_exact_content_type_filter(tmp_path, db_session: AsyncSession): + svc = _service(tmp_path, db_session) + await _seed(svc) + + items, total = await svc.list_files(content_type="application/pdf") + assert [i.filename for i in items] == ["q3-report.pdf"] + assert total == 1 + + +async def test_trailing_slash_selects_a_whole_family(tmp_path, db_session: AsyncSession): + """`image/` has to catch png and jpeg, or the filter is useless on a + bucket holding nine kinds of image.""" + svc = _service(tmp_path, db_session) + await _seed(svc) + + items, total = await svc.list_files(content_type="image/") + assert sorted(i.filename for i in items) == ["banner.jpeg", "logo.png"] + assert total == 2 + + +async def test_search_and_type_filters_combine(tmp_path, db_session: AsyncSession): + svc = _service(tmp_path, db_session) + await _seed(svc) + + items, total = await svc.list_files(search="o", content_type="image/png") + assert [i.filename for i in items] == ["logo.png"] + assert total == 1 + + +async def test_total_reflects_the_filter_not_the_table(tmp_path, db_session: AsyncSession): + """A total that ignores the filter yields pages that render empty.""" + svc = _service(tmp_path, db_session) + await _seed(svc) + + _, unfiltered = await svc.list_files() + _, filtered = await svc.list_files(search="report") + assert unfiltered == 4 + assert filtered == 1 + + +async def test_facets_report_present_types_with_counts(tmp_path, db_session: AsyncSession): + svc = _service(tmp_path, db_session) + await _seed(svc) + + facets = {f["value"]: f["count"] for f in await svc.content_type_facets()} + assert facets == { + "application/pdf": 1, + "text/csv": 1, + "image/png": 1, + "image/jpeg": 1, + } + + +async def test_facets_are_empty_for_an_empty_bucket(tmp_path, db_session: AsyncSession): + svc = _service(tmp_path, db_session) + assert await svc.content_type_facets() == [] + + +async def test_deleted_files_leave_the_facets(tmp_path, db_session: AsyncSession): + """Offering a type filter that matches nothing is a dead end.""" + svc = _service(tmp_path, db_session) + out = await svc.upload(_upload("q3-report.pdf", "application/pdf")) + await svc.upload(_upload("logo.png", "image/png")) + + await svc.delete(out.id) + + facets = {f["value"] for f in await svc.content_type_facets()} + assert facets == {"image/png"} diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 04226d0d..a634993a 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -178,6 +178,11 @@ export default { 'file_storage.browse.description': '', 'file_storage.browse.empty_description': '', 'file_storage.browse.empty_title': '', + 'file_storage.browse.next': '', + 'file_storage.browse.no_match_description': '', + 'file_storage.browse.no_match_title': '', + 'file_storage.browse.page_of': '', + 'file_storage.browse.previous': '', 'file_storage.browse.title': '', 'file_storage.browse.upload_button': '', 'file_storage.browse.uploading': '', @@ -189,6 +194,9 @@ export default { 'file_storage.errors.bad_type': '', 'file_storage.errors.not_found': '', 'file_storage.errors.too_large': '', + 'file_storage.filters.search_placeholder': '', + 'file_storage.filters.type_all': '', + 'file_storage.filters.type_label': '', 'file_storage.table.actions': '', 'file_storage.table.filename': '', 'file_storage.table.size': '', @@ -198,7 +206,12 @@ export default { 'file_storage.toasts.delete_failed': '', 'file_storage.toasts.deleted': '', 'file_storage.toasts.upload_failed': '', + 'file_storage.toasts.upload_failed_named': '', 'file_storage.toasts.uploaded': '', + 'file_storage.toasts.uploaded_count_one': '', + 'file_storage.toasts.uploaded_count_other': '', + 'file_storage.upload.dismiss': '', + 'file_storage.upload.in_progress': '', 'host.error.correlation_id_copy': '', 'host.error.correlation_id_label': '', 'host.error.forbidden_description': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index b5d14234..ec01fe00 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -239,6 +239,11 @@ export const keys = { description: 'file_storage.browse.description', empty_description: 'file_storage.browse.empty_description', empty_title: 'file_storage.browse.empty_title', + next: 'file_storage.browse.next', + no_match_description: 'file_storage.browse.no_match_description', + no_match_title: 'file_storage.browse.no_match_title', + page_of: 'file_storage.browse.page_of', + previous: 'file_storage.browse.previous', title: 'file_storage.browse.title', upload_button: 'file_storage.browse.upload_button', uploading: 'file_storage.browse.uploading', @@ -255,6 +260,11 @@ export const keys = { not_found: 'file_storage.errors.not_found', too_large: 'file_storage.errors.too_large', }, + filters: { + search_placeholder: 'file_storage.filters.search_placeholder', + type_all: 'file_storage.filters.type_all', + type_label: 'file_storage.filters.type_label', + }, table: { actions: 'file_storage.table.actions', filename: 'file_storage.table.filename', @@ -267,7 +277,15 @@ export const keys = { delete_failed: 'file_storage.toasts.delete_failed', deleted: 'file_storage.toasts.deleted', upload_failed: 'file_storage.toasts.upload_failed', + upload_failed_named: 'file_storage.toasts.upload_failed_named', uploaded: 'file_storage.toasts.uploaded', + uploaded_count: 'file_storage.toasts.uploaded_count', + uploaded_count_one: 'file_storage.toasts.uploaded_count_one', + uploaded_count_other: 'file_storage.toasts.uploaded_count_other', + }, + upload: { + dismiss: 'file_storage.upload.dismiss', + in_progress: 'file_storage.upload.in_progress', }, }, host: { From d84b0b051f155932161ddc1181ad8c9404b12d09 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 11 Aug 2026 14:28:07 +0200 Subject: [PATCH 03/17] feat(audit_log): resolve actor and entity ids to records MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit log showed `user_id` as a bare uuid and left `entity_id` unlinked, so a row proved something changed without saying who did it or offering any route to the record. Actors now resolve to a display name (full name, else email) via one batched query per page — an audit page is 50 rows authored by a handful of admins. Deleted accounts keep showing the raw id rather than blanking the row: the id is still the truthful record of who acted. Entity ids link to the owning module's screen through a new AuditLinkRegistry and `register_audit_links` hook, following the existing register_design_packs precedent. Modules declare their own table -> URL mapping, so audit_log never learns about anyone else's routes. Users, Settings and BackgroundTasks register theirs; tables with no per-record screen (join rows, stored files) simply render unlinked, which the registry treats as normal rather than an error. Resolution happens at render time only — the stored row keeps the bare id, which is what makes it durable. Links widen no access: the target route enforces its own permissions. --- framework/core/simple_module_core/__init__.py | 3 + .../core/simple_module_core/audit_links.py | 79 +++++++++++++++++ framework/core/simple_module_core/module.py | 23 +++++ framework/core/simple_module_core/services.py | 2 + framework/core/tests/test_audit_links.py | 57 ++++++++++++ framework/core/tests/test_services.py | 3 + .../simple_module_hosting/app_builder.py | 4 + .../audit_log/audit_log/endpoints/views.py | 21 ++++- modules/audit_log/audit_log/locales/en.json | 3 +- modules/audit_log/audit_log/pages/Browse.tsx | 69 +++++++++++++-- modules/audit_log/audit_log/resolve.py | 74 ++++++++++++++++ .../audit_log/tests/test_audit_log_resolve.py | 88 +++++++++++++++++++ .../background_tasks/module.py | 14 +++ modules/settings/settings/module.py | 11 +++ .../settings/tests/test_module_settings.py | 1 + modules/users/users/module.py | 10 +++ packages/i18n/src/generated-resources.ts | 1 + packages/i18n/src/keys.generated.ts | 1 + 18 files changed, 455 insertions(+), 9 deletions(-) create mode 100644 framework/core/simple_module_core/audit_links.py create mode 100644 framework/core/tests/test_audit_links.py create mode 100644 modules/audit_log/audit_log/resolve.py create mode 100644 modules/audit_log/tests/test_audit_log_resolve.py diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index d43e1bfa..b1de5332 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -1,5 +1,6 @@ """SimpleModule Core - Module system, menu, permissions, events, and diagnostics.""" +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry from simple_module_core.design_packs import DesignPack, DesignPackRegistry from simple_module_core.diagnostics import ( DiagnosticLevel, @@ -45,6 +46,8 @@ "DEFAULT_AUTH_PROVIDER", "FRAMEWORK_API_VERSION", "CircularDependencyError", + "AuditLink", + "AuditLinkRegistry", "DesignPack", "DesignPackRegistry", "DiagnosticLevel", diff --git a/framework/core/simple_module_core/audit_links.py b/framework/core/simple_module_core/audit_links.py new file mode 100644 index 00000000..44df6687 --- /dev/null +++ b/framework/core/simple_module_core/audit_links.py @@ -0,0 +1,79 @@ +"""Audit-link registry — modules teach the audit log how to reach their records. + +An audit entry stores the table name and primary key of the row that changed +(``files_file``, ``a91f3c2b…``). That is enough to prove what happened and +useless for doing anything about it: the reader has an id and no way to open +the record it names. + +A module declares where its rows live via +:meth:`~simple_module_core.module.ModuleBase.register_audit_links`; the host +collects them into one registry at boot and stores it on +``app.state.audit_links``. + +**The registry maps table names to URL templates, nothing more.** It does not +verify the row exists or that the reader may open it — following a link to a +deleted record lands on that screen's own 404, and permissions are enforced by +the target route as usual. +""" + +from __future__ import annotations + +from dataclasses import dataclass + +_ID_PLACEHOLDER = "{id}" + + +@dataclass(frozen=True) +class AuditLink: + """Where the records of one audited table can be viewed. + + Args: + entity_type: The audited table name, matching ``AuditEntry.entity_type`` + (e.g. ``"users_user"``). + url_template: Path containing ``{id}``, substituted with the entity id + (e.g. ``"/admin/users/{id}/edit"``). + label: Human-readable name for the entity kind, shown instead of the + raw table name (e.g. ``"User"``). + """ + + entity_type: str + url_template: str + label: str = "" + + def __post_init__(self) -> None: + if _ID_PLACEHOLDER not in self.url_template: + raise ValueError( + f"AuditLink for {self.entity_type!r} has url_template " + f"{self.url_template!r}, which contains no {_ID_PLACEHOLDER} — " + f"every row would link to the same page" + ) + + def url_for(self, entity_id: str) -> str: + return self.url_template.replace(_ID_PLACEHOLDER, entity_id) + + +class AuditLinkRegistry: + """Aggregates every module's :class:`AuditLink` declarations. + + Populated once during boot (``register_audit_links`` hook) and read + thereafter by the audit log view when it renders each row. + """ + + def __init__(self) -> None: + self._links: dict[str, AuditLink] = {} + + def register(self, link: AuditLink) -> None: + existing = self._links.get(link.entity_type) + if existing is not None and existing != link: + raise ValueError( + f"Two modules claim audit links for {link.entity_type!r}: " + f"{existing.url_template!r} and {link.url_template!r}" + ) + self._links[link.entity_type] = link + + def get(self, entity_type: str) -> AuditLink | None: + return self._links.get(entity_type) + + @property + def all_links(self) -> dict[str, AuditLink]: + return dict(self._links) diff --git a/framework/core/simple_module_core/module.py b/framework/core/simple_module_core/module.py index 703c04e6..04d8d233 100644 --- a/framework/core/simple_module_core/module.py +++ b/framework/core/simple_module_core/module.py @@ -10,6 +10,7 @@ if TYPE_CHECKING: from fastapi import APIRouter, FastAPI + from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.events import EventBus from simple_module_core.feature_flags import FeatureFlagRegistry @@ -163,6 +164,28 @@ def register_design_packs(self, registry): order. """ + def register_audit_links(self, registry: AuditLinkRegistry) -> None: + """Declare where this module's audited records can be viewed. + + An audit entry stores a table name and a primary key, which tells the + reader what changed but gives them no way to open it. Override this + hook to make your module's rows reachable from the audit log:: + + def register_audit_links(self, registry): + registry.register( + AuditLink( + entity_type="users_user", + url_template="/admin/users/{id}/edit", + label="User", + ) + ) + + ``entity_type`` is the ``__tablename__`` the rows are audited under. + Registering only supplies the URL — the target route still enforces + its own permissions, so linking never widens access. Called once at + boot, in dependency order. + """ + def register_middleware(self, app: FastAPI) -> None: """Add middleware to the application. diff --git a/framework/core/simple_module_core/services.py b/framework/core/simple_module_core/services.py index cb86c11c..4df8860c 100644 --- a/framework/core/simple_module_core/services.py +++ b/framework/core/simple_module_core/services.py @@ -20,6 +20,7 @@ from simple_module_db.session import DatabaseState from simple_module_hosting.settings import Settings + from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.events import EventBus from simple_module_core.feature_flags import FeatureFlagRegistry @@ -44,6 +45,7 @@ class Services: health_registry: HealthRegistry public_routes: PublicRouteRegistry design_packs: DesignPackRegistry + audit_links: AuditLinkRegistry i18n_registry: I18nRegistry inertia_config: InertiaConfig modules: tuple[ModuleBase, ...] diff --git a/framework/core/tests/test_audit_links.py b/framework/core/tests/test_audit_links.py new file mode 100644 index 00000000..d9e734af --- /dev/null +++ b/framework/core/tests/test_audit_links.py @@ -0,0 +1,57 @@ +"""Tests for AuditLink / AuditLinkRegistry. + +The audit log stores a table name and a primary key. This registry is how a +module says where those rows can be opened, so the log stops being a wall of +unactionable uuids. +""" + +from __future__ import annotations + +import pytest +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry + + +class TestAuditLink: + def test_url_for_substitutes_the_id(self): + link = AuditLink(entity_type="users_user", url_template="/users/admin/{id}") + assert link.url_for("a91f3c2b") == "/users/admin/a91f3c2b" + + def test_template_without_placeholder_is_rejected(self): + """Every row would otherwise link to the same page.""" + with pytest.raises(ValueError, match="contains no"): + AuditLink(entity_type="users_user", url_template="/users/admin") + + def test_label_defaults_to_empty(self): + assert AuditLink(entity_type="t", url_template="/t/{id}").label == "" + + +class TestAuditLinkRegistry: + def test_get_returns_none_for_unclaimed_tables(self): + """Join tables and blob stores have no screen; that is not an error.""" + assert AuditLinkRegistry().get("permissions_user_permission") is None + + def test_register_and_get(self): + reg = AuditLinkRegistry() + link = AuditLink(entity_type="users_user", url_template="/users/admin/{id}", label="User") + reg.register(link) + assert reg.get("users_user") is link + + def test_conflicting_claims_raise(self): + """Two modules mapping one table means one of them silently loses.""" + reg = AuditLinkRegistry() + reg.register(AuditLink(entity_type="users_user", url_template="/a/{id}")) + with pytest.raises(ValueError, match="Two modules claim"): + reg.register(AuditLink(entity_type="users_user", url_template="/b/{id}")) + + def test_registering_the_same_link_twice_is_allowed(self): + """Idempotent re-registration must not break a re-entrant boot.""" + reg = AuditLinkRegistry() + for _ in range(2): + reg.register(AuditLink(entity_type="users_user", url_template="/a/{id}")) + assert len(reg.all_links) == 1 + + def test_all_links_is_a_copy(self): + reg = AuditLinkRegistry() + reg.register(AuditLink(entity_type="users_user", url_template="/a/{id}")) + reg.all_links.clear() + assert reg.get("users_user") is not None diff --git a/framework/core/tests/test_services.py b/framework/core/tests/test_services.py index 384a598d..15e9da67 100644 --- a/framework/core/tests/test_services.py +++ b/framework/core/tests/test_services.py @@ -31,6 +31,7 @@ async def test_services_round_trip_field_access(self) -> None: assert s.health_registry is _SENTINEL_HEALTH assert s.public_routes is _SENTINEL_PUBLIC_ROUTES assert s.design_packs is _SENTINEL_DESIGN_PACKS + assert s.audit_links is _SENTINEL_AUDIT_LINKS assert s.i18n_registry is _SENTINEL_I18N assert s.inertia_config is _SENTINEL_INERTIA assert s.modules == () @@ -45,6 +46,7 @@ async def test_services_round_trip_field_access(self) -> None: _SENTINEL_HEALTH = object() _SENTINEL_PUBLIC_ROUTES = object() _SENTINEL_DESIGN_PACKS = object() +_SENTINEL_AUDIT_LINKS = object() _SENTINEL_I18N = object() _SENTINEL_INERTIA = object() @@ -61,6 +63,7 @@ def _make_services() -> Services: health_registry=_SENTINEL_HEALTH, # type: ignore[arg-type] public_routes=_SENTINEL_PUBLIC_ROUTES, # type: ignore[arg-type] design_packs=_SENTINEL_DESIGN_PACKS, # type: ignore[arg-type] + audit_links=_SENTINEL_AUDIT_LINKS, # type: ignore[arg-type] i18n_registry=_SENTINEL_I18N, # type: ignore[arg-type] inertia_config=_SENTINEL_INERTIA, # type: ignore[arg-type] modules=(), diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 70c96f4a..916854ab 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -10,6 +10,7 @@ from pathlib import Path from fastapi import FastAPI +from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.diagnostics import DiagnosticLevel, print_diagnostics, run_diagnostics from simple_module_core.discovery import discover_modules, select_auth_provider, topological_sort @@ -173,6 +174,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: health_registry = HealthRegistry() public_route_registry = PublicRouteRegistry() design_pack_registry = DesignPackRegistry() + audit_link_registry = AuditLinkRegistry() @asynccontextmanager async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: @@ -228,6 +230,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: mod.register_health_checks(health_registry) mod.register_public_routes(public_route_registry) mod.register_design_packs(design_pack_registry) + mod.register_audit_links(audit_link_registry) # Stop attributing to the last module in the loop — anything registered # after this point belongs to no module in particular. @@ -293,6 +296,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: health_registry=health_registry, public_routes=public_route_registry, design_packs=design_pack_registry, + audit_links=audit_link_registry, i18n_registry=i18n_registry, inertia_config=inertia_config, modules=tuple(modules), diff --git a/modules/audit_log/audit_log/endpoints/views.py b/modules/audit_log/audit_log/endpoints/views.py index ed29a419..acb31854 100644 --- a/modules/audit_log/audit_log/endpoints/views.py +++ b/modules/audit_log/audit_log/endpoints/views.py @@ -4,10 +4,12 @@ from datetime import datetime -from fastapi import APIRouter, Depends, Query +from fastapi import APIRouter, Depends, Query, Request from inertia import InertiaResponse +from simple_module_db.deps import get_db from simple_module_hosting.inertia_deps import InertiaDep from simple_module_hosting.permissions import RequiresPermission +from sqlalchemy.ext.asyncio import AsyncSession from audit_log.constants import ( DEFAULT_PAGE_SIZE, @@ -16,6 +18,7 @@ PERM_VIEW, ) from audit_log.deps import AuditLogServiceDep +from audit_log.resolve import entity_link, resolve_actors router = APIRouter() @@ -36,8 +39,10 @@ def _safe_int(raw: str | None, default: int) -> int: dependencies=[Depends(RequiresPermission(PERM_VIEW))], ) async def browse( + request: Request, inertia: InertiaDep, service: AuditLogServiceDep, + db: AsyncSession = Depends(get_db), entity_type: str | None = Query(default=None), entity_id: str | None = Query(default=None), action: str | None = Query(default=None), @@ -62,10 +67,22 @@ async def browse( ) entity_types = await service.distinct_entity_types() + # Resolve ids for display only — the stored row keeps the bare id, which + # is what makes it a durable record. + actors = await resolve_actors(db, [item.user_id for item in result.items]) + links = request.app.state.sm.audit_links + + items = [] + for item in result.items: + payload = item.model_dump(mode="json") + payload["actor"] = actors.get(item.user_id or "") + payload["entity"] = entity_link(links, item.entity_type, item.entity_id) + items.append(payload) + return await inertia.render( PAGE_BROWSE, { - "items": [item.model_dump(mode="json") for item in result.items], + "items": items, "total": result.total, "page": result.page, "page_size": result.page_size, diff --git a/modules/audit_log/audit_log/locales/en.json b/modules/audit_log/audit_log/locales/en.json index 691e0f81..7d3296dc 100644 --- a/modules/audit_log/audit_log/locales/en.json +++ b/modules/audit_log/audit_log/locales/en.json @@ -38,6 +38,7 @@ "show_more": "Show {count} more…", "show_less": "Show less", "system_user": "System", - "no_changes": "—" + "no_changes": "—", + "deleted_user": "This account no longer exists" } } diff --git a/modules/audit_log/audit_log/pages/Browse.tsx b/modules/audit_log/audit_log/pages/Browse.tsx index 558a91b7..dd534a99 100644 --- a/modules/audit_log/audit_log/pages/Browse.tsx +++ b/modules/audit_log/audit_log/pages/Browse.tsx @@ -1,5 +1,6 @@ -import { Head, router, usePage } from '@inertiajs/react'; +import { Head, Link, router, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; +import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Button } from '@simple-module-py/ui/components/ui/button'; @@ -24,6 +25,12 @@ interface Change { new?: unknown; } +interface EntityRef { + /** null when no module claims this table — the id renders unlinked. */ + url: string | null; + label: string; +} + interface AuditEntryRead { id: string; entity_type: string; @@ -31,6 +38,9 @@ interface AuditEntryRead { action: 'created' | 'updated' | 'deleted' | 'soft_deleted'; changes: Change[]; user_id: string | null; + /** Display name resolved from user_id, or null for deleted/system actors. */ + actor: string | null; + entity: EntityRef; correlation_id: string | null; created_at: string; } @@ -99,6 +109,56 @@ function ChangesList({ entry }: { entry: AuditEntryRead }) { ); } +/** + * Entity kind and id. The id is a link when the owning module registered one, + * and always copyable — quoting an id into a ticket is the other thing people + * do with this column. + */ +function EntityCell({ entry }: { entry: AuditEntryRead }) { + const label = entry.entity?.label ?? entry.entity_type; + const url = entry.entity?.url ?? null; + const shortId = entry.entity_id.length > 12 ? `${entry.entity_id.slice(0, 8)}…` : entry.entity_id; + + return ( +
+ {label} + {url ? ( + + {shortId} + + ) : ( + + )} +
+ ); +} + +/** Who acted: display name where the account still exists, raw id otherwise. */ +function ActorCell({ entry }: { entry: AuditEntryRead }) { + const { t } = useT(); + if (!entry.user_id) return <>{t(keys.audit_log.changes.system_user)}; + if (entry.actor) { + return ( + + {entry.actor} + + ); + } + // The account is gone. The id is still the truthful record of who acted, + // so show it rather than pretending the action had no author. + return ( + + ); +} + function Browse() { const { items, total, page, page_size, entity_types, filters } = usePage<{ props: Props }>() .props as unknown as Props; @@ -193,13 +253,10 @@ function Browse() { - {entry.entity_type} - - {entry.entity_id} - + - {entry.user_id ?? t(keys.audit_log.changes.system_user)} + diff --git a/modules/audit_log/audit_log/resolve.py b/modules/audit_log/audit_log/resolve.py new file mode 100644 index 00000000..06421f28 --- /dev/null +++ b/modules/audit_log/audit_log/resolve.py @@ -0,0 +1,74 @@ +"""Turn the raw ids in an audit entry into something a reader can act on. + +An entry stores ``user_id`` and ``entity_id`` as bare primary keys. That is +correct for a permanent record — display names change, ids do not — but it +leaves the screen showing two uuids per row and no route to either record. + +This module resolves both at render time: + +* ``resolve_actors`` batches one query for every user id on the page. +* ``entity_link`` consults the host's :class:`AuditLinkRegistry`, which each + module populates with the URL template for its own tables. + +Neither is stored. The audit row keeps the id it recorded. +""" + +from __future__ import annotations + +import uuid +from typing import Any + +from simple_module_core.audit_links import AuditLinkRegistry +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession +from users.models import User + + +async def resolve_actors(db: AsyncSession, user_ids: list[str | None]) -> dict[str, str]: + """Map user id -> display label for every id on the page. + + One query for the whole page rather than one per row: an audit page is 50 + entries and the same handful of admins tend to appear on all of them. + + Ids that no longer resolve (deleted accounts) are simply absent from the + result — the caller falls back to showing the raw id, which is still the + truthful record of who acted. + """ + wanted = {uid for uid in user_ids if uid} + if not wanted: + return {} + + # Audit rows store the id as text; the users table keys on UUID. Ids that + # aren't parseable belong to some other id space, so skip rather than + # fail the page. + parsed: dict[uuid.UUID, str] = {} + for raw in wanted: + try: + parsed[uuid.UUID(raw)] = raw + except (ValueError, AttributeError, TypeError): + continue + if not parsed: + return {} + + rows = ( + await db.execute(select(User.id, User.email, User.full_name).where(User.id.in_(parsed))) + ).all() + + resolved: dict[str, str] = {} + for user_id, email, full_name in rows: + raw = parsed.get(user_id) or str(user_id) + resolved[raw] = full_name or email + return resolved + + +def entity_link(registry: AuditLinkRegistry, entity_type: str, entity_id: str) -> dict[str, Any]: + """Return ``{"url", "label"}`` for one entity reference. + + ``url`` is ``None`` when no module claims this table — the id still + renders, just without a link, which is the correct outcome for tables that + have no screen of their own (join rows, stored files). + """ + link = registry.get(entity_type) + if link is None: + return {"url": None, "label": entity_type} + return {"url": link.url_for(entity_id), "label": link.label or entity_type} diff --git a/modules/audit_log/tests/test_audit_log_resolve.py b/modules/audit_log/tests/test_audit_log_resolve.py new file mode 100644 index 00000000..dda40b82 --- /dev/null +++ b/modules/audit_log/tests/test_audit_log_resolve.py @@ -0,0 +1,88 @@ +"""Resolving the raw ids an audit entry stores into names and links. + +The browse screen showed `user_id` as a bare uuid and left `entity_id` +unlinked, so an entry told you something changed but not who or where. +""" + +from __future__ import annotations + +import uuid + +from audit_log.resolve import entity_link, resolve_actors +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry +from sqlalchemy.ext.asyncio import AsyncSession +from users.models import User + + +def _registry() -> AuditLinkRegistry: + reg = AuditLinkRegistry() + reg.register( + AuditLink(entity_type="users_user", url_template="/users/admin/{id}", label="User") + ) + return reg + + +class TestEntityLink: + def test_registered_table_resolves_to_a_url(self): + ref = entity_link(_registry(), "users_user", "a91") + assert ref == {"url": "/users/admin/a91", "label": "User"} + + def test_unclaimed_table_renders_unlinked(self): + """Join rows and blob stores have no screen — the id still shows.""" + ref = entity_link(_registry(), "permissions_user_permission", "x1") + assert ref["url"] is None + assert ref["label"] == "permissions_user_permission" + + def test_missing_label_falls_back_to_the_table_name(self): + reg = AuditLinkRegistry() + reg.register(AuditLink(entity_type="files_file", url_template="/f/{id}")) + assert entity_link(reg, "files_file", "z")["label"] == "files_file" + + +class TestResolveActors: + async def test_resolves_full_name_when_present(self, db_session: AsyncSession): + user = User( + email="sam@example.com", + hashed_password="x", + full_name="Sam Carter", + is_active=True, + ) + db_session.add(user) + await db_session.flush() + + resolved = await resolve_actors(db_session, [str(user.id)]) + assert resolved[str(user.id)] == "Sam Carter" + + async def test_falls_back_to_email_without_a_name(self, db_session: AsyncSession): + user = User(email="noname@example.com", hashed_password="x", is_active=True) + db_session.add(user) + await db_session.flush() + + resolved = await resolve_actors(db_session, [str(user.id)]) + assert resolved[str(user.id)] == "noname@example.com" + + async def test_unknown_ids_are_simply_absent(self, db_session: AsyncSession): + """A deleted account must not blank the row — the caller shows the id.""" + missing = str(uuid.uuid4()) + assert await resolve_actors(db_session, [missing]) == {} + + async def test_empty_and_none_ids_short_circuit(self, db_session: AsyncSession): + assert await resolve_actors(db_session, [None, None]) == {} + assert await resolve_actors(db_session, []) == {} + + async def test_unparseable_ids_do_not_fail_the_page(self, db_session: AsyncSession): + """System actors from another id space must not 500 the audit log.""" + assert await resolve_actors(db_session, ["celery-worker-1"]) == {} + + async def test_batches_the_whole_page_in_one_pass(self, db_session: AsyncSession): + users = [ + User(email=f"u{i}@example.com", hashed_password="x", full_name=f"U{i}", is_active=True) + for i in range(3) + ] + db_session.add_all(users) + await db_session.flush() + + ids = [str(u.id) for u in users] + # Repeats are normal: one admin usually authors most of a page. + resolved = await resolve_actors(db_session, ids + ids) + assert set(resolved) == set(ids) diff --git a/modules/background_tasks/background_tasks/module.py b/modules/background_tasks/background_tasks/module.py index 7c7c934c..0b07c89a 100644 --- a/modules/background_tasks/background_tasks/module.py +++ b/modules/background_tasks/background_tasks/module.py @@ -8,6 +8,7 @@ from typing import TYPE_CHECKING from fastapi import APIRouter +from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry @@ -69,6 +70,19 @@ def register_settings(self, app: FastAPI) -> None: def register_permissions(self, registry: PermissionRegistry) -> None: registry.add_group(PERM_GROUP, [PERM_VIEW, PERM_MANAGE]) + def register_audit_links(self, registry: AuditLinkRegistry) -> None: + from simple_module_core.audit_links import AuditLink + + from background_tasks.constants import TABLE_TASK_EXECUTION + + registry.register( + AuditLink( + entity_type=TABLE_TASK_EXECUTION, + url_template=f"{VIEW_PREFIX}/{{id}}", + label="Task execution", + ) + ) + def register_menu_items(self, registry: MenuRegistry) -> None: registry.add( MenuItem( diff --git a/modules/settings/settings/module.py b/modules/settings/settings/module.py index b60e0c97..3d12f768 100644 --- a/modules/settings/settings/module.py +++ b/modules/settings/settings/module.py @@ -6,6 +6,7 @@ from pathlib import Path from fastapi import APIRouter, FastAPI +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry @@ -21,6 +22,7 @@ MODULE_NAME, MODULE_PACKAGE, PERM_GROUP, + TABLE_SETTING, VIEW_PREFIX, ) @@ -73,6 +75,15 @@ def register_menu_items(self, registry: MenuRegistry) -> None: def register_permissions(self, registry: PermissionRegistry) -> None: registry.add_group(PERM_GROUP, list(ALL_PERMISSIONS)) + def register_audit_links(self, registry: AuditLinkRegistry) -> None: + registry.register( + AuditLink( + entity_type=TABLE_SETTING, + url_template=f"{VIEW_PREFIX}/{{id}}/edit", + label="Setting", + ) + ) + def locale_dirs(self) -> dict[str, Path]: base = Path(str(importlib.resources.files(__package__) / "locales")) return {LOCALE_NAMESPACE: base} diff --git a/modules/settings/tests/test_module_settings.py b/modules/settings/tests/test_module_settings.py index ab71e154..29c9410c 100644 --- a/modules/settings/tests/test_module_settings.py +++ b/modules/settings/tests/test_module_settings.py @@ -49,6 +49,7 @@ def test_collect_exposes_type_requires_restart_group(): health_registry=None, # type: ignore[arg-type] public_routes=None, # type: ignore[arg-type] design_packs=None, # type: ignore[arg-type] + audit_links=None, # type: ignore[arg-type] i18n_registry=None, # type: ignore[arg-type] inertia_config=None, # type: ignore[arg-type] modules=(_DemoModule(),), # type: ignore[arg-type] diff --git a/modules/users/users/module.py b/modules/users/users/module.py index dd56d200..30a7e642 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -5,6 +5,7 @@ from typing import TYPE_CHECKING from fastapi import APIRouter, Depends +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry @@ -96,6 +97,15 @@ def register_permissions(self, registry: PermissionRegistry) -> None: ) registry.map_role(USER_ROLE_NAME, [PERM_USERS_SELF_PROFILE]) + def register_audit_links(self, registry: AuditLinkRegistry) -> None: + registry.register( + AuditLink( + entity_type="users_user", + url_template=f"{_URL_USERS_ADMIN}/{{id}}", + label="User", + ) + ) + def register_menu_items(self, registry: MenuRegistry) -> None: # Admin-only user management registry.add( diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index a634993a..f4ad33fc 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -13,6 +13,7 @@ export default { 'audit_log.browse.previous': '', 'audit_log.browse.showing': '', 'audit_log.browse.title': '', + 'audit_log.changes.deleted_user': '', 'audit_log.changes.fields_set': '', 'audit_log.changes.no_changes': '', 'audit_log.changes.show_less': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index ec01fe00..9c63eba0 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -19,6 +19,7 @@ export const keys = { title: 'audit_log.browse.title', }, changes: { + deleted_user: 'audit_log.changes.deleted_user', fields_set: 'audit_log.changes.fields_set', no_changes: 'audit_log.changes.no_changes', show_less: 'audit_log.changes.show_less', From 9e0f8cdc3e03f3c290d3f14354bb61b79a3d841c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 11 Aug 2026 14:34:00 +0200 Subject: [PATCH 04/17] feat(permissions,feature_flags): honest inherited state, tenant picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1n — the user-grants switch reflected only the direct grant, which is correct (it is the only thing the form can change) but it was also the row's only signal. A permission the user genuinely held through a role rendered identically to one they did not hold, with a small badge as the sole clue. Rows now answer two questions separately: a leading indicator says whether the user has the permission at all, and the switch says whether it is granted here. The badge names the granting role, because "inherited" does not tell an admin which role to edit — `inherited_by` maps each key to its sources, and deliberately includes keys that are also direct, so a permission granted both ways cannot look purely direct. 1r — the flags scope was a free-text tenant id, where a typo silently showed an empty scope rather than an error. Replaced with a picker over the tenants that have overrides. The list cannot be closed — the framework has no tenant registry, ids arrive on auth claims — so naming a new tenant by hand stays possible for creating a tenant's first override. The currently-viewed tenant is folded into the options, or it would vanish from its own picker. --- .../feature_flags/locales/en.json | 7 +- .../feature_flags/pages/Browse.tsx | 58 +-------- .../pages/components/TenantPicker.tsx | 112 ++++++++++++++++++ .../permissions/contracts/schemas.py | 4 + .../permissions/endpoints/views.py | 1 + .../permissions/permissions/locales/en.json | 9 +- .../permissions/pages/UserEdit.tsx | 56 +++------ .../pages/components/PermissionRow.tsx | 83 +++++++++++++ modules/permissions/permissions/service.py | 20 ++++ .../test_permissions_inheritance_sources.py | 92 ++++++++++++++ packages/i18n/src/generated-resources.ts | 12 +- packages/i18n/src/keys.generated.ts | 13 +- 12 files changed, 358 insertions(+), 109 deletions(-) create mode 100644 modules/feature_flags/feature_flags/pages/components/TenantPicker.tsx create mode 100644 modules/permissions/permissions/pages/components/PermissionRow.tsx create mode 100644 modules/permissions/tests/test_permissions_inheritance_sources.py diff --git a/modules/feature_flags/feature_flags/locales/en.json b/modules/feature_flags/feature_flags/locales/en.json index fd5b7908..36e573c7 100644 --- a/modules/feature_flags/feature_flags/locales/en.json +++ b/modules/feature_flags/feature_flags/locales/en.json @@ -10,12 +10,11 @@ "viewing_tenant": "Viewing overrides for tenant \"{tenant_id}\". Tenant overrides beat the system value.", "tenant_id_label": "Tenant ID", "tenant_id_placeholder": "e.g. acme", - "scope_label": "Scope", "scope_system": "System (all tenants)", - "scope_tenant": "Specific tenant", "go": "View", - "back_to_system": "Back to system view", - "tenants_with_overrides": "Tenants with overrides" + "viewing_label": "Viewing", + "scope_other": "Other tenant…", + "cancel": "Cancel" }, "table": { "name": "Flag", diff --git a/modules/feature_flags/feature_flags/pages/Browse.tsx b/modules/feature_flags/feature_flags/pages/Browse.tsx index 5dda3df2..583e98bc 100644 --- a/modules/feature_flags/feature_flags/pages/Browse.tsx +++ b/modules/feature_flags/feature_flags/pages/Browse.tsx @@ -10,7 +10,6 @@ import { EmptyMedia, EmptyTitle, } from '@simple-module-py/ui/components/ui/empty'; -import { Input } from '@simple-module-py/ui/components/ui/input'; import { Switch } from '@simple-module-py/ui/components/ui/switch'; import { Table, @@ -23,8 +22,8 @@ import { import { usePermissions } from '@simple-module-py/ui/hooks/use-permissions'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; import { Flag, RotateCcw } from 'lucide-react'; -import { useState } from 'react'; import { toast } from 'sonner'; +import { TenantPicker } from './components/TenantPicker'; interface FeatureFlag { name: string; @@ -55,7 +54,6 @@ function Browse() { const { t } = useT(); const { can } = usePermissions(); const canManage = can('feature_flags.manage'); - const [tenantInput, setTenantInput] = useState(tenant_id ?? ''); function handleToggle(flag: FeatureFlag, next: boolean) { router.post( @@ -86,10 +84,6 @@ function Browse() { ); } - function visitTenant(value: string) { - router.visit(buildPath(value.trim() || null)); - } - return ( <> @@ -98,51 +92,11 @@ function Browse() { description={t(keys.feature_flags.browse.description)} > -
{ - e.preventDefault(); - visitTenant(tenantInput); - }} - > -
- - setTenantInput(e.target.value)} - placeholder={t(keys.feature_flags.browse.tenant_id_placeholder)} - /> -
- - {tenant_id && ( - - )} -
- {tenants.length > 0 && ( -
- - {t(keys.feature_flags.browse.tenants_with_overrides)}: - - {tenants.map((tid) => ( - - ))} -
- )} + router.visit(buildPath(next))} + />

{tenant_id ? t(keys.feature_flags.browse.viewing_tenant, { tenant_id }) diff --git a/modules/feature_flags/feature_flags/pages/components/TenantPicker.tsx b/modules/feature_flags/feature_flags/pages/components/TenantPicker.tsx new file mode 100644 index 00000000..b1dc5120 --- /dev/null +++ b/modules/feature_flags/feature_flags/pages/components/TenantPicker.tsx @@ -0,0 +1,112 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Button } from '@simple-module-py/ui/components/ui/button'; +import { Input } from '@simple-module-py/ui/components/ui/input'; +import { + Select, + SelectContent, + SelectItem, + SelectSeparator, + SelectTrigger, + SelectValue, +} from '@simple-module-py/ui/components/ui/select'; +import { useState } from 'react'; + +interface Props { + /** Currently-viewed tenant, or null for the system scope. */ + tenantId: string | null; + /** Tenants that already carry at least one override. */ + tenants: string[]; + onSelect: (tenantId: string | null) => void; +} + +const SYSTEM = '__system__'; +const CUSTOM = '__custom__'; + +/** + * Scope picker for the flags table. + * + * This was a free-text box, so viewing a tenant meant knowing and correctly + * typing its id — and a typo silently showed a scope with no overrides + * instead of an error. + * + * The list cannot be closed: there is no tenant registry in the framework + * (ids arrive on auth claims), so the only tenants this app can enumerate are + * those that already have an override. Creating the *first* override for a + * tenant therefore still needs a way to name one by hand, which is what the + * "other tenant" branch is for. + */ +export function TenantPicker({ tenantId, tenants, onSelect }: Props) { + const { t } = useT(); + const [custom, setCustom] = useState(false); + const [draft, setDraft] = useState(''); + + // The tenant being viewed may not have overrides yet, so it can be absent + // from `tenants` — without this it would vanish from its own picker. + const options = [...new Set(tenantId ? [...tenants, tenantId] : tenants)].sort(); + + function handleChange(value: string) { + if (value === CUSTOM) { + setCustom(true); + setDraft(''); + return; + } + setCustom(false); + onSelect(value === SYSTEM ? null : value); + } + + return ( +

+
+ + {t(keys.feature_flags.browse.viewing_label)} + + +
+ + {custom && ( +
{ + e.preventDefault(); + const trimmed = draft.trim(); + if (trimmed) onSelect(trimmed); + }} + > +
+ + setDraft(e.target.value)} + placeholder={t(keys.feature_flags.browse.tenant_id_placeholder)} + /> +
+ + +
+ )} +
+ ); +} diff --git a/modules/permissions/permissions/contracts/schemas.py b/modules/permissions/permissions/contracts/schemas.py index 29eee230..d3e0a314 100644 --- a/modules/permissions/permissions/contracts/schemas.py +++ b/modules/permissions/permissions/contracts/schemas.py @@ -57,6 +57,10 @@ class UserPermissionsOut(SQLModel): """Keys granted directly to this user.""" inherited: list[str] """Keys the user holds via any of their roles (excluding duplicates of ``direct``).""" + inherited_by: dict[str, list[str]] = Field(default_factory=dict) + """Every role-granted key mapped to the roles granting it — including keys + that are also granted directly, so the UI can distinguish "role only" from + "role and direct". Naming the role is what makes the grant actionable.""" class UserPermissionsUpdate(SQLModel): diff --git a/modules/permissions/permissions/endpoints/views.py b/modules/permissions/permissions/endpoints/views.py index 14e6fa95..fe2840eb 100644 --- a/modules/permissions/permissions/endpoints/views.py +++ b/modules/permissions/permissions/endpoints/views.py @@ -99,6 +99,7 @@ async def edit_user( "roles": assignment.roles, "direct": assignment.direct, "inherited": assignment.inherited, + "inherited_by": assignment.inherited_by, "groups": [g.model_dump(mode="json") for g in groups], }, ) diff --git a/modules/permissions/permissions/locales/en.json b/modules/permissions/permissions/locales/en.json index ebc10f6c..ae194a81 100644 --- a/modules/permissions/permissions/locales/en.json +++ b/modules/permissions/permissions/locales/en.json @@ -49,13 +49,16 @@ "roles_label": "Roles", "no_roles": "No roles assigned", "empty": "No permissions have been registered by any installed module.", - "inherited_badge": "inherited", - "inherited_hint": "Already granted via a role the user holds.", + "inherited_hint": "Granted by role: {roles}. Turning the switch off here will not revoke it — edit the role instead.", "submit_button": "Save changes", "reset_button": "Discard", "cancel_link": "Back", "direct_summary": "Direct", - "effective_summary": "Effective" + "effective_summary": "Effective", + "via_role": "via {role}", + "effective_yes": "This user has this permission", + "effective_no": "This user does not have this permission", + "direct_toggle_label": "Grant {key} directly to this user" }, "errors": { "role_not_found": "Role not found.", diff --git a/modules/permissions/permissions/pages/UserEdit.tsx b/modules/permissions/permissions/pages/UserEdit.tsx index 96ec0ac8..8cab7e74 100644 --- a/modules/permissions/permissions/pages/UserEdit.tsx +++ b/modules/permissions/permissions/pages/UserEdit.tsx @@ -6,12 +6,12 @@ import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; -import { Switch } from '@simple-module-py/ui/components/ui/switch'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; import { Check, KeyRound, Link2, Package, Search, ShieldCheck } from 'lucide-react'; import type React from 'react'; import { useMemo, useState } from 'react'; import { toast } from 'sonner'; +import { PermissionRow } from './components/PermissionRow'; type Group = { name: string; permissions: string[] }; type UserProp = { id: string; email: string; full_name: string | null }; @@ -21,17 +21,18 @@ type Props = { roles: string[]; direct: string[]; inherited: string[]; + /** Permission key -> roles granting it, so a row can name its source. */ + inherited_by: Record; groups: Group[]; }; -function UserEdit({ user, roles, direct, inherited, groups }: Props) { +function UserEdit({ user, roles, direct, inherited, inherited_by: inheritedBy, groups }: Props) { const { t } = useT(); const { data, setData, put, processing, isDirty, reset } = useForm<{ permissions: string[] }>({ permissions: direct, }); const [q, setQ] = useState(''); - const inheritedSet = useMemo(() => new Set(inherited), [inherited]); const directSet = useMemo(() => new Set(data.permissions), [data.permissions]); const effectiveSet = useMemo( () => new Set([...data.permissions, ...inherited]), @@ -172,43 +173,18 @@ function UserEdit({ user, roles, direct, inherited, groups }: Props) {
- {group.permissions.map((key, i) => { - const fromRole = inheritedSet.has(key); - const checked = directSet.has(key); - return ( - - ); - })} + {group.permissions.map((key, i) => ( + + ))}
); diff --git a/modules/permissions/permissions/pages/components/PermissionRow.tsx b/modules/permissions/permissions/pages/components/PermissionRow.tsx new file mode 100644 index 00000000..1a600343 --- /dev/null +++ b/modules/permissions/permissions/pages/components/PermissionRow.tsx @@ -0,0 +1,83 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Badge } from '@simple-module-py/ui/components/ui/badge'; +import { Switch } from '@simple-module-py/ui/components/ui/switch'; +import { Check, Minus } from 'lucide-react'; + +interface Props { + permissionKey: string; + /** Granted directly to this user — the only thing the switch controls. */ + direct: boolean; + /** Roles granting this key, empty when none do. */ + viaRoles: string[]; + onToggle: (key: string, checked: boolean) => void; + className?: string; +} + +/** + * One permission, showing *effective* access separately from the direct grant. + * + * The switch reflects only the direct grant, which is correct — it is the only + * thing this form can change. Previously that was also the row's only signal, + * so a permission the user genuinely holds through a role rendered as "off". + * The leading indicator now answers "does this user have it?" and the switch + * answers "is it granted here?", which are different questions. + */ +export function PermissionRow({ + permissionKey, + direct, + viaRoles, + onToggle, + className = '', +}: Props) { + const { t } = useT(); + const inherited = viaRoles.length > 0; + const effective = direct || inherited; + const switchId = `perm-${permissionKey}`; + + return ( +
+ + + + {permissionKey} + + + {inherited && ( + + {t(keys.permissions.user_edit.via_role, { role: viaRoles[0] })} + {viaRoles.length > 1 ? ` +${viaRoles.length - 1}` : ''} + + )} + + onToggle(permissionKey, c === true)} + aria-label={t(keys.permissions.user_edit.direct_toggle_label, { key: permissionKey })} + title={t(keys.permissions.user_edit.direct_toggle_label, { key: permissionKey })} + /> +
+ ); +} diff --git a/modules/permissions/permissions/service.py b/modules/permissions/permissions/service.py index 7601618c..5755440a 100644 --- a/modules/permissions/permissions/service.py +++ b/modules/permissions/permissions/service.py @@ -176,6 +176,7 @@ async def get_user_permissions(self, user_id: uuid.UUID) -> UserPermissionsOut | roles=sorted(role_names), direct=sorted(direct), inherited=sorted(inherited), + inherited_by=self._resolve_role_sources(role_names), ) async def set_user_permissions( @@ -212,6 +213,7 @@ async def set_user_permissions( roles=sorted(role_names), direct=sorted(wanted), inherited=sorted(inherited), + inherited_by=self._resolve_role_sources(role_names), ) # ── Effective-permissions resolution ─────────────────────── @@ -229,6 +231,24 @@ def _resolve_role_permissions(self, role_names: list[str]) -> set[str]: resolved.update(perms) return resolved + def _resolve_role_sources(self, role_names: list[str]) -> dict[str, list[str]]: + """Map each inherited permission key to the roles that grant it. + + "Inherited" alone doesn't tell an admin what to change — they need to + know *which* role to edit. Two roles can grant the same key, so the + value is a list. + """ + from simple_module_core.permissions import WILDCARD + + role_map = self.registry.role_map + sources: dict[str, list[str]] = {} + for name in sorted(role_names): + perms = role_map.get(name, []) + keys = self.registry.all_permissions if WILDCARD in perms else perms + for key in keys: + sources.setdefault(key, []).append(name) + return sources + async def resolve_effective_permissions(self, user_id: uuid.UUID) -> set[str]: """Return every permission key the user holds (role-inherited + direct).""" user = await self._get_user(user_id) diff --git a/modules/permissions/tests/test_permissions_inheritance_sources.py b/modules/permissions/tests/test_permissions_inheritance_sources.py new file mode 100644 index 00000000..5eca8c49 --- /dev/null +++ b/modules/permissions/tests/test_permissions_inheritance_sources.py @@ -0,0 +1,92 @@ +"""Naming which role grants an inherited permission. + +The user-grants screen drove its switch off `direct` alone, so a permission +the user genuinely holds through a role rendered exactly like one they did +not hold. `inherited_by` gives each row its source, which is also what tells +an admin which role to edit if they want the permission gone. +""" + +from __future__ import annotations + +from permissions.service import PermissionService +from simple_module_core.permissions import WILDCARD, PermissionRegistry +from sqlalchemy.ext.asyncio import AsyncSession + + +def _registry() -> PermissionRegistry: + reg = PermissionRegistry() + reg.add_group("Products", ["products.view", "products.create"]) + reg.add_group("Settings", ["settings.read", "settings.manage"]) + return reg + + +def _service(db_session: AsyncSession, reg: PermissionRegistry) -> PermissionService: + return PermissionService(db_session, reg) + + +class TestResolveRoleSources: + def test_maps_each_key_to_its_granting_role(self, db_session: AsyncSession): + reg = _registry() + reg.map_role("editor", ["products.view", "products.create"]) + svc = _service(db_session, reg) + + sources = svc._resolve_role_sources(["editor"]) + assert sources["products.view"] == ["editor"] + assert sources["products.create"] == ["editor"] + + def test_two_roles_granting_one_key_both_appear(self, db_session: AsyncSession): + """Revoking via one role would not be enough; the admin needs both.""" + reg = _registry() + reg.map_role("editor", ["products.view"]) + reg.map_role("viewer", ["products.view"]) + svc = _service(db_session, reg) + + assert svc._resolve_role_sources(["viewer", "editor"])["products.view"] == [ + "editor", + "viewer", + ] + + def test_wildcard_role_claims_every_registered_key(self, db_session: AsyncSession): + """An admin role holds everything — each row must still say why.""" + reg = _registry() + reg.map_role("admin", [WILDCARD]) + svc = _service(db_session, reg) + + sources = svc._resolve_role_sources(["admin"]) + assert set(sources) == set(reg.all_permissions) + assert sources["settings.manage"] == ["admin"] + + def test_no_roles_yields_nothing(self, db_session: AsyncSession): + assert _service(db_session, _registry())._resolve_role_sources([]) == {} + + def test_unknown_role_contributes_nothing(self, db_session: AsyncSession): + assert _service(db_session, _registry())._resolve_role_sources(["ghost"]) == {} + + +class TestUserPermissionsPayload: + async def test_inherited_by_covers_keys_that_are_also_direct( + self, db_session: AsyncSession + ) -> None: + """`inherited` drops direct duplicates; `inherited_by` must not, or a + key granted both ways looks purely direct and revoking it silently + leaves the role grant in place.""" + from users.constants import USER_ROLE_ID + from users.models import Role, User + + reg = _registry() + reg.map_role("editor", ["products.view"]) + + role = Role(id=USER_ROLE_ID, name="editor", description="") + user = User(email="e@example.com", hashed_password="x", is_active=True) + user.roles = [role] + db_session.add_all([role, user]) + await db_session.flush() + + svc = _service(db_session, reg) + await svc.set_user_permissions(user.id, ["products.view"]) + out = await svc.get_user_permissions(user.id) + + assert out is not None + assert "products.view" in out.direct + assert "products.view" not in out.inherited + assert out.inherited_by["products.view"] == ["editor"] diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index f4ad33fc..4b08d2db 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -140,20 +140,19 @@ export default { 'dashboard.home.title': '', 'dashboard.home.welcome_card_title': '', 'dashboard.home.welcome_message': '', - 'feature_flags.browse.back_to_system': '', + 'feature_flags.browse.cancel': '', 'feature_flags.browse.count_one': '', 'feature_flags.browse.count_other': '', 'feature_flags.browse.description': '', 'feature_flags.browse.empty_description': '', 'feature_flags.browse.empty_title': '', 'feature_flags.browse.go': '', - 'feature_flags.browse.scope_label': '', + 'feature_flags.browse.scope_other': '', 'feature_flags.browse.scope_system': '', - 'feature_flags.browse.scope_tenant': '', 'feature_flags.browse.tenant_id_label': '', 'feature_flags.browse.tenant_id_placeholder': '', - 'feature_flags.browse.tenants_with_overrides': '', 'feature_flags.browse.title': '', + 'feature_flags.browse.viewing_label': '', 'feature_flags.browse.viewing_system': '', 'feature_flags.browse.viewing_tenant': '', 'feature_flags.table.actions': '', @@ -285,15 +284,18 @@ export default { 'permissions.user_edit.cancel_link': '', 'permissions.user_edit.description': '', 'permissions.user_edit.direct_summary': '', + 'permissions.user_edit.direct_toggle_label': '', + 'permissions.user_edit.effective_no': '', 'permissions.user_edit.effective_summary': '', + 'permissions.user_edit.effective_yes': '', 'permissions.user_edit.empty': '', - 'permissions.user_edit.inherited_badge': '', 'permissions.user_edit.inherited_hint': '', 'permissions.user_edit.no_roles': '', 'permissions.user_edit.reset_button': '', 'permissions.user_edit.roles_label': '', 'permissions.user_edit.submit_button': '', 'permissions.user_edit.title': '', + 'permissions.user_edit.via_role': '', 'settings.browse.delete_confirm': '', 'settings.browse.delete_link': '', 'settings.browse.edit_link': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 9c63eba0..2aa03a4d 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -189,7 +189,7 @@ export const keys = { }, feature_flags: { browse: { - back_to_system: 'feature_flags.browse.back_to_system', + cancel: 'feature_flags.browse.cancel', count: 'feature_flags.browse.count', count_one: 'feature_flags.browse.count_one', count_other: 'feature_flags.browse.count_other', @@ -197,13 +197,13 @@ export const keys = { empty_description: 'feature_flags.browse.empty_description', empty_title: 'feature_flags.browse.empty_title', go: 'feature_flags.browse.go', - scope_label: 'feature_flags.browse.scope_label', + scope: 'feature_flags.browse.scope', + scope_other: 'feature_flags.browse.scope_other', scope_system: 'feature_flags.browse.scope_system', - scope_tenant: 'feature_flags.browse.scope_tenant', tenant_id_label: 'feature_flags.browse.tenant_id_label', tenant_id_placeholder: 'feature_flags.browse.tenant_id_placeholder', - tenants_with_overrides: 'feature_flags.browse.tenants_with_overrides', title: 'feature_flags.browse.title', + viewing_label: 'feature_flags.browse.viewing_label', viewing_system: 'feature_flags.browse.viewing_system', viewing_tenant: 'feature_flags.browse.viewing_tenant', }, @@ -381,15 +381,18 @@ export const keys = { cancel_link: 'permissions.user_edit.cancel_link', description: 'permissions.user_edit.description', direct_summary: 'permissions.user_edit.direct_summary', + direct_toggle_label: 'permissions.user_edit.direct_toggle_label', + effective_no: 'permissions.user_edit.effective_no', effective_summary: 'permissions.user_edit.effective_summary', + effective_yes: 'permissions.user_edit.effective_yes', empty: 'permissions.user_edit.empty', - inherited_badge: 'permissions.user_edit.inherited_badge', inherited_hint: 'permissions.user_edit.inherited_hint', no_roles: 'permissions.user_edit.no_roles', reset_button: 'permissions.user_edit.reset_button', roles_label: 'permissions.user_edit.roles_label', submit_button: 'permissions.user_edit.submit_button', title: 'permissions.user_edit.title', + via_role: 'permissions.user_edit.via_role', }, }, settings: { From f3d97b7f5d4ecda56021791641b715be90a10cb7 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 11 Aug 2026 14:36:38 +0200 Subject: [PATCH 05/17] feat(branding): preview the sidebar and banner live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preview was a logo tile and the app name, so the two surfaces branding is most visible on — the sidebar every authenticated page carries, and the site-wide banner — could only be checked by saving and waiting for a reload. The preview now renders both from form state, so it updates as you type. It uses the same near-black `bg-app-sidebar` token as the real shell, so the dark logo variant is judged against the surface it will actually sit on, and it lists the viewer's own sidebar entries rather than invented ones. The sidebar mark is rendered inline instead of through BrandingMark: that component takes its badge colour as a Tailwind class, and the preview has to show whatever hex is currently in the colour field. The logo tile is kept below — the sidebar shows the dark variant, so it is the only place the light-surface logo appears. --- .../pages/components/StatusStrip.tsx | 2 +- .../branding/components/BrandingPreview.tsx | 153 ++++++++++++++++++ modules/branding/branding/locales/en.json | 4 +- modules/branding/branding/pages/Manage.tsx | 35 ++-- .../file_storage/pages/Browse.tsx | 13 +- packages/i18n/src/generated-resources.ts | 2 + packages/i18n/src/keys.generated.ts | 2 + 7 files changed, 179 insertions(+), 32 deletions(-) create mode 100644 modules/branding/branding/components/BrandingPreview.tsx diff --git a/modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx b/modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx index 38bf40cd..a1d6b523 100644 --- a/modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx +++ b/modules/background_tasks/background_tasks/pages/components/StatusStrip.tsx @@ -1,4 +1,4 @@ -import { type TaskStatus, TASK_STATUS } from '../constants'; +import { TASK_STATUS, type TaskStatus } from '../constants'; import { statusLabel } from './ExecutionRow'; export type StatusCounts = Partial>; diff --git a/modules/branding/branding/components/BrandingPreview.tsx b/modules/branding/branding/components/BrandingPreview.tsx new file mode 100644 index 00000000..6558cf29 --- /dev/null +++ b/modules/branding/branding/components/BrandingPreview.tsx @@ -0,0 +1,153 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Card, CardContent, CardHeader, CardTitle } from '@simple-module-py/ui/components/ui/card'; +import type { MenuItem } from '@simple-module-py/ui/types'; + +export type PreviewSeverity = 'info' | 'warning' | 'danger'; + +interface Props { + appName: string; + /** Hex colour from the form, or '' to fall back to the default swatch. */ + color: string; + defaultColor: string; + logoUrl: string | null; + /** Dark-surface logo variant; falls back to `logoUrl` like the real sidebar. */ + logoDarkUrl: string | null; + bannerMessage: string; + bannerSeverity: PreviewSeverity; + /** The viewer's own sidebar entries, so the preview shows a real nav. */ + menuItems: MenuItem[]; +} + +/** + * Mirrors BrandingBanner's severity map. Duplicated rather than imported + * because that component reads the *saved* banner from shared props — the + * whole point here is to render the unsaved form state. + */ +const SEVERITY_CLASS: Record = { + info: 'bg-sky-600 text-white', + warning: 'bg-amber-500 text-black', + danger: 'bg-red-600 text-white', +}; + +const MAX_NAV_ROWS = 5; + +/** + * Live preview of the sidebar and banner as the form is edited. + * + * Previously the preview was a logo tile and the app name, so the two places + * branding is actually most visible — the sidebar every authenticated page + * carries, and the site-wide banner — could only be checked by saving and + * waiting for a full reload. Everything here is driven by form state, so it + * updates as you type and never needs a round trip. + */ +export function BrandingPreview({ + appName, + color, + defaultColor, + logoUrl, + logoDarkUrl, + bannerMessage, + bannerSeverity, + menuItems, +}: Props) { + const { t } = useT(); + const accent = color || defaultColor; + const name = appName || 'SimpleModule'; + const initial = name.trim()[0]?.toUpperCase() ?? 'S'; + // Same fallback the real sidebar uses: no dark variant means the primary + // logo has to hold up against the near-black surface. + const darkLogo = logoDarkUrl ?? logoUrl; + const rows = menuItems.slice(0, MAX_NAV_ROWS); + + return ( + + + {t(keys.branding.manage.preview_title)} + + +
+ {bannerMessage ? ( +
+ {bannerMessage} +
+ ) : ( +
+ {t(keys.branding.manage.preview_no_banner)} +
+ )} + +
+ {/* Sidebar. `bg-app-sidebar` is the same near-black token the real + shell uses, so the dark-variant logo is judged against the + surface it will actually sit on. */} +
+ {/* Rendered inline rather than via BrandingMark: that component + takes its badge colour as a Tailwind class, and the point + here is to show the hex currently in the colour field. */} +
+ {darkLogo ? ( + {name} + ) : ( + + {initial} + + )} + {name} +
+
+ {rows.map((item) => ( +
+ {item.label} +
+ ))} + {rows.length === 0 && ( +
—
+ )} +
+
+ +
+
+
+
+
+ {t(keys.branding.manage.preview_button)} +
+
+
+
+ + {/* The original logo-tile preview: the light-surface logo, which the + sidebar above cannot show because it renders the dark variant. */} +
+
+ {logoUrl ? ( + {name} + ) : ( + {name.trim()[0]?.toUpperCase() ?? 'S'} + )} +
+ {name} +
+ + + ); +} diff --git a/modules/branding/branding/locales/en.json b/modules/branding/branding/locales/en.json index 5b996d6d..2fab0c2d 100644 --- a/modules/branding/branding/locales/en.json +++ b/modules/branding/branding/locales/en.json @@ -47,6 +47,8 @@ "preview_title": "Preview", "saved_toast": "Branding updated", "error_toast": "Could not update branding", - "upload_error_toast": "Could not upload image" + "upload_error_toast": "Could not upload image", + "preview_no_banner": "No banner set", + "preview_button": "Action" } } diff --git a/modules/branding/branding/pages/Manage.tsx b/modules/branding/branding/pages/Manage.tsx index f379faa2..616c2b34 100644 --- a/modules/branding/branding/pages/Manage.tsx +++ b/modules/branding/branding/pages/Manage.tsx @@ -16,6 +16,7 @@ import type { SharedProps } from '@simple-module-py/ui/types'; import { useEffect, useState } from 'react'; import { toast } from 'sonner'; import { BannerField, type BannerSeverity } from '../components/BannerField'; +import { BrandingPreview } from '../components/BrandingPreview'; import { DesignPackField, type DesignPackOption } from '../components/DesignPackField'; import { FooterCard, type FooterPayload } from '../components/FooterCard'; import { ImageField } from '../components/ImageField'; @@ -264,30 +265,16 @@ function Manage() { onSave={saveFooter} /> - - - {t(keys.branding.manage.preview_title)} - - -
-
- {branding?.logoUrl ? ( - {appName} - ) : ( - {(appName.trim()[0] ?? 'S').toUpperCase()} - )} -
- {appName || 'SimpleModule'} -
- - +
diff --git a/modules/file_storage/file_storage/pages/Browse.tsx b/modules/file_storage/file_storage/pages/Browse.tsx index 79f96ef3..a27b3f34 100644 --- a/modules/file_storage/file_storage/pages/Browse.tsx +++ b/modules/file_storage/file_storage/pages/Browse.tsx @@ -71,7 +71,12 @@ function formatBytes(n: number): string { function Browse() { const page = usePage<{ props: Props }>(); - const { files, pagination, filters, content_types: contentTypes } = page.props as unknown as Props; + const { + files, + pagination, + filters, + content_types: contentTypes, + } = page.props as unknown as Props; const { t } = useT(); const { can } = usePermissions(); const canUpload = can(PERMISSIONS.UPLOAD); @@ -139,11 +144,7 @@ function Browse() { - + {files.map((file) => ( {file.filename} diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 4b08d2db..dc9b942d 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -114,6 +114,8 @@ export default { 'branding.manage.logo_label': '', 'branding.manage.preset_help': '', 'branding.manage.preset_label': '', + 'branding.manage.preview_button': '', + 'branding.manage.preview_no_banner': '', 'branding.manage.preview_title': '', 'branding.manage.primary_color_help': '', 'branding.manage.primary_color_label': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 2aa03a4d..56d45ed7 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -149,6 +149,8 @@ export const keys = { logo_label: 'branding.manage.logo_label', preset_help: 'branding.manage.preset_help', preset_label: 'branding.manage.preset_label', + preview_button: 'branding.manage.preview_button', + preview_no_banner: 'branding.manage.preview_no_banner', preview_title: 'branding.manage.preview_title', primary_color_help: 'branding.manage.primary_color_help', primary_color_label: 'branding.manage.primary_color_label', From e6054fe520f1f636ceadbac66961f4ca3f11acdc Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 11 Aug 2026 14:48:43 +0200 Subject: [PATCH 06/17] feat(settings): lead with module forms, key autocomplete, value provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1o — /settings/ was the raw key/value store: a database view, keyed by dotted strings, shown to anyone who clicked "Settings". The per-module forms now own the section root and the store moves to /settings/store. /settings/modules redirects (308) so existing links and bookmarks keep working. 1p — the setting key was free text, and a typo produced a row that looked saved and was silently never read: a failure mode with no feedback at all. The field now suggests every . an installed module declares, and selecting one also fills in its declared value type. It stays free text — a module can read keys this screen cannot enumerate — so an unrecognised key gets an advisory warning rather than a validation error. 1q — a field showed its value and its env var name but never which one was in force. Each field now reports its source (stored override / environment / default), mirroring hydrate_settings' real precedence, and calls out the genuinely confusing case: a stored override silently shadowing a set env var. "Test connection" runs the module's health checks on demand rather than inventing a parallel mechanism, so settings never learns what SMTP or S3 is. Users and FileStorage gain the checks that makes this real: an SMTP session that authenticates and hangs up without sending, and a HEAD for a key that cannot exist. Both re-read live settings on every run rather than pinning the boot-time instance, and both report the reason — "connection refused" and "authentication failed" call for different fixes. --- modules/dashboard/tests/test_view_routes.py | 16 ++- modules/file_storage/file_storage/health.py | 42 ++++++ modules/file_storage/file_storage/module.py | 11 ++ modules/settings/settings/_module_settings.py | 62 +++++++- modules/settings/settings/constants.py | 6 + modules/settings/settings/endpoints/views.py | 136 ++++++++++++++++-- modules/settings/settings/locales/en.json | 15 +- modules/settings/settings/pages/Create.tsx | 30 ++-- .../settings/settings/pages/ModulesEdit.tsx | 10 +- .../settings/pages/components/FieldInput.tsx | 6 + .../settings/pages/components/FieldSource.tsx | 59 ++++++++ .../settings/pages/components/KeyField.tsx | 92 ++++++++++++ .../settings/pages/components/ModuleForm.tsx | 30 ++-- .../pages/components/TestConnectionButton.tsx | 69 +++++++++ modules/settings/settings/pages/routes.ts | 7 +- .../tests/test_settings_field_sources.py | 101 +++++++++++++ modules/users/users/health.py | 49 +++++++ modules/users/users/mailer/smtp.py | 22 +++ modules/users/users/module.py | 12 ++ packages/i18n/src/generated-resources.ts | 11 ++ packages/i18n/src/keys.generated.ts | 11 ++ 21 files changed, 745 insertions(+), 52 deletions(-) create mode 100644 modules/file_storage/file_storage/health.py create mode 100644 modules/settings/settings/pages/components/FieldSource.tsx create mode 100644 modules/settings/settings/pages/components/KeyField.tsx create mode 100644 modules/settings/settings/pages/components/TestConnectionButton.tsx create mode 100644 modules/settings/tests/test_settings_field_sources.py create mode 100644 modules/users/users/health.py diff --git a/modules/dashboard/tests/test_view_routes.py b/modules/dashboard/tests/test_view_routes.py index ebf25c14..83034d0a 100644 --- a/modules/dashboard/tests/test_view_routes.py +++ b/modules/dashboard/tests/test_view_routes.py @@ -29,20 +29,28 @@ async def test_dashboard_doctor_renders_for_admin(authenticated_client): @pytest.mark.anyio async def test_settings_index_renders_for_admin(authenticated_client): - """The Settings module's browse page is reachable for admins.""" + """The section root now leads with the per-module forms.""" resp = await authenticated_client.get("/settings/", follow_redirects=False) assert resp.status_code == 200, resp.text assert "data-page" in resp.text @pytest.mark.anyio -async def test_settings_modules_renders_for_admin(authenticated_client): - """Per-module settings UI must render without error.""" - resp = await authenticated_client.get("/settings/modules", follow_redirects=False) +async def test_settings_store_renders_for_admin(authenticated_client): + """The raw key/value store moved off the root but stays reachable.""" + resp = await authenticated_client.get("/settings/store", follow_redirects=False) assert resp.status_code == 200, resp.text assert "data-page" in resp.text +@pytest.mark.anyio +async def test_settings_modules_url_still_resolves(authenticated_client): + """Existing links and bookmarks to /settings/modules must not break.""" + resp = await authenticated_client.get("/settings/modules", follow_redirects=False) + assert resp.status_code == 308, resp.text + assert resp.headers["location"].endswith("/settings/") + + @pytest.mark.anyio async def test_dashboard_index_redirects_anon_to_login(client): """An unauthenticated visit to ``/dashboard/`` must redirect to login. diff --git a/modules/file_storage/file_storage/health.py b/modules/file_storage/file_storage/health.py new file mode 100644 index 00000000..5a7b83c7 --- /dev/null +++ b/modules/file_storage/file_storage/health.py @@ -0,0 +1,42 @@ +"""Health check for the configured storage backend. + +Doubles as the "Test connection" action on the module-settings screen. A +misconfigured bucket otherwise stays invisible until the first upload fails, +which is usually a user's upload rather than the admin's. +""" + +from __future__ import annotations + +import uuid + +from fastapi import FastAPI +from simple_module_core.health import HealthCheckResult, HealthStatus + +CHECK_BACKEND = "file_storage.backend" + +# A key that cannot exist. `exists()` on a missing key is the cheapest call +# that still proves credentials, region, and bucket name are all correct — +# a HEAD, with nothing written and nothing to clean up. +_PROBE_PREFIX = "__healthcheck__/" + + +def build_backend_check(app: FastAPI): + """Return an async check closing over *app* so it re-reads the live backend.""" + + async def check() -> HealthCheckResult: + services = getattr(app.state, "file_storage", None) + backend = getattr(services, "backend", None) + if backend is None: + return HealthCheckResult( + status=HealthStatus.UNHEALTHY, detail="No storage backend configured" + ) + + try: + await backend.exists(f"{_PROBE_PREFIX}{uuid.uuid4()}") + except Exception as exc: + return HealthCheckResult(status=HealthStatus.UNHEALTHY, detail=str(exc)) + return HealthCheckResult( + status=HealthStatus.HEALTHY, detail=f"{type(backend).__name__} reachable" + ) + + return check diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index fadd0766..72c4187f 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -129,3 +129,14 @@ async def on_startup(self, app: FastAPI) -> None: settings.s3_region, settings.s3_endpoint_url or "(default)", ) + + # Registered here, not in register_health_checks: the backend does not + # exist until this hook builds it, and the check must follow later + # settings changes rather than pinning the boot-time instance. + from simple_module_core.health import HealthCheck + + from file_storage.health import CHECK_BACKEND, build_backend_check + + app.state.sm.health_registry.add( + HealthCheck(name=CHECK_BACKEND, check=build_backend_check(app), module=self.meta.name) + ) diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index ad3c8176..953f6e73 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -6,6 +6,7 @@ from __future__ import annotations +import os import re from dataclasses import dataclass from typing import Any @@ -41,6 +42,26 @@ class ModuleSettingField: type: str requires_restart: bool group: str | None + env_set: bool = False + """The field's ``SM_*`` env var is present in the process environment.""" + db_override: bool = False + """A stored setting overrides this field.""" + + @property + def source(self) -> str: + """Where the live value came from: ``db``, ``env`` or ``default``. + + Mirrors the precedence in ``hydrate_settings``: DB overrides are passed + to the constructor explicitly, so they beat env, which pydantic reads + for anything left unset, which in turn beats the field default. Showing + this is the difference between "why is this not taking effect" being a + five-minute question and an afternoon. + """ + if self.db_override: + return "db" + if self.env_set: + return "env" + return "default" @dataclass(frozen=True, slots=True) @@ -93,16 +114,22 @@ def _resolve_default(info) -> Any: return None -def _field_view(name: str, settings: BaseSettings, prefix: str) -> ModuleSettingField: +def _field_view( + name: str, + settings: BaseSettings, + prefix: str, + overridden: frozenset[str] = frozenset(), +) -> ModuleSettingField: cls = type(settings) info = cls.model_fields[name] raw_value = getattr(settings, name) secret = is_secret_field(name) extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} default = _resolve_default(info) + env_var = f"{prefix}{name.upper()}" return ModuleSettingField( name=name, - env_var=f"{prefix}{name.upper()}", + env_var=env_var, value=_mask(raw_value) if secret else raw_value, default=_mask(default) if secret else default, description=info.description or "", @@ -110,16 +137,26 @@ def _field_view(name: str, settings: BaseSettings, prefix: str) -> ModuleSetting type=value_type_for_field(cls, name), requires_restart=bool(extra.get("requires_restart", False)), group=extra.get("group"), + env_set=env_var in os.environ, + db_override=name in overridden, ) -def collect_module_settings(app: FastAPI) -> list[ModuleSettingsView]: +def collect_module_settings( + app: FastAPI, + overrides: dict[str, frozenset[str]] | None = None, +) -> list[ModuleSettingsView]: """Return a sorted, serializable view of every module's BaseSettings. Folds in both ``app.state.sm.modules`` (plugin modules) and additional packages registered via ``app.state.settings.module_registry`` (e.g. ``"host"``) that aren't backed by a ``ModuleBase`` instance. + + ``overrides`` maps package -> field names carrying a stored override. It + is passed in rather than read here because fetching it is async and this + function is not; callers without it get ``db_override=False`` throughout. """ + by_package = overrides or {} views: list[ModuleSettingsView] = [] seen: set[str] = set() @@ -128,7 +165,7 @@ def collect_module_settings(app: FastAPI) -> list[ModuleSettingsView]: settings = _extract_settings(app, package) if settings is None: continue - views.append(_build_view(mod.meta.name, package, settings)) + views.append(_build_view(mod.meta.name, package, settings, by_package)) seen.add(package) settings_services = getattr(app.state, "settings", None) @@ -140,16 +177,24 @@ def collect_module_settings(app: FastAPI) -> list[ModuleSettingsView]: settings = _extract_settings(app, package) if settings is None: continue - views.append(_build_view(package.title(), package, settings)) + views.append(_build_view(package.title(), package, settings, by_package)) seen.add(package) views.sort(key=lambda v: v.module_name) return views -def _build_view(module_name: str, package: str, settings: BaseSettings) -> ModuleSettingsView: +def _build_view( + module_name: str, + package: str, + settings: BaseSettings, + overrides: dict[str, frozenset[str]] | None = None, +) -> ModuleSettingsView: prefix = env_prefix_for(package) - fields = [_field_view(name, settings, prefix) for name in type(settings).model_fields] + overridden = (overrides or {}).get(package, frozenset()) + fields = [ + _field_view(name, settings, prefix, overridden) for name in type(settings).model_fields + ] return ModuleSettingsView( module_name=module_name, package=package, @@ -178,6 +223,9 @@ def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: "type": f.type, "requires_restart": f.requires_restart, "group": f.group, + "env_set": f.env_set, + "db_override": f.db_override, + "source": f.source, } for f in v.fields ], diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index b389f628..69dc9cf1 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -48,6 +48,12 @@ VIEW_CREATE_PATH: Final = "/create" VIEW_EDIT_PATH: Final = "/{setting_id}/edit" VIEW_MODULES_PATH: Final = "/modules" +"""Legacy path for the per-module forms. Those now live at the section root; +this redirects, so existing links and bookmarks keep working.""" + +VIEW_STORE_PATH: Final = "/store" +"""Raw key/value store. Demoted from the section root: it is a database view, +and an admin looking for "settings" almost always wants the module forms.""" API_BY_ID_PATH: Final = "/{setting_id}" API_BY_KEY_PATH: Final = "/by-key/{key}" API_RESOLVE_PATH: Final = "/resolve/{key}" diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 4de333f9..7488ee05 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -9,7 +9,7 @@ from __future__ import annotations -from fastapi import APIRouter, Depends, Request +from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse from pydantic import ValidationError from simple_module_hosting.inertia_deps import InertiaDep @@ -26,6 +26,7 @@ VIEW_CREATE_PATH, VIEW_EDIT_PATH, VIEW_MODULES_PATH, + VIEW_STORE_PATH, ) from settings.contracts.schemas import SettingCreate, SettingUpdate from settings.deps import get_setting_service @@ -36,16 +37,25 @@ _PAGE_EDIT = "Settings/Edit" _PAGE_MODULES_EDIT = "Settings/ModulesEdit" -_REDIRECT_SETTINGS = "/settings" +# Row-level actions return to the raw store they were performed in, not to +# the module forms that now own the section root. +_REDIRECT_SETTINGS = "/settings/store" +_REDIRECT_MODULES = "/settings/" router = APIRouter() -@router.get("/", response_model=None) +@router.get(VIEW_STORE_PATH, response_model=None) async def browse( inertia: InertiaDep, service: SettingService = Depends(get_setting_service), ) -> InertiaResponse: + """The raw key/value store. + + Moved off the section root: it is a database view, and an admin who clicks + "Settings" is nearly always after a module's form, not a table of rows + keyed by dotted strings. + """ items = await service.list_all() return await inertia.render( _PAGE_BROWSE, @@ -53,9 +63,38 @@ async def browse( ) +@router.get(VIEW_MODULES_PATH, response_model=None) +async def modules_redirect() -> RedirectResponse: + """The per-module forms moved to the section root; keep old links alive.""" + return RedirectResponse(_REDIRECT_MODULES, status_code=308) + + @router.get(VIEW_CREATE_PATH, response_model=None) -async def create_view(inertia: InertiaDep) -> InertiaResponse: - return await inertia.render(_PAGE_CREATE) +async def create_view(request: Request, inertia: InertiaDep) -> InertiaResponse: + return await inertia.render(_PAGE_CREATE, {"known_keys": _known_keys(request)}) + + +def _known_keys(request: Request) -> list[dict[str, str]]: + """Every ``.`` a module actually reads, for autocomplete. + + The key field is free text, and a typo produces a row that looks saved and + is silently never read — the failure gives no feedback at all. Suggesting + the registered keys makes the common case unmissable without forbidding + the uncommon one: keys outside this list stay valid, since a module can + read settings the settings module cannot enumerate. + """ + suggestions: list[dict[str, str]] = [] + for view in collect_module_settings(request.app): + for field in view.fields: + suggestions.append( + { + "key": f"{view.package}.{field.name}", + "type": field.type, + "description": field.description, + "module": view.module_name, + } + ) + return sorted(suggestions, key=lambda s: s["key"]) @router.get(VIEW_EDIT_PATH, response_model=None) @@ -73,7 +112,9 @@ async def edit_view( # ── Form actions (POST/PUT/DELETE → redirect) ───────────────── -@router.post("/", response_model=None) +# Posts to the store collection, which is where the rows live now that the +# section root renders the module forms. +@router.post(VIEW_STORE_PATH, response_model=None) async def create_action( request: Request, service: SettingService = Depends(get_setting_service), @@ -111,14 +152,89 @@ async def delete_action( return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) -@router.get(VIEW_MODULES_PATH, response_model=None) -async def modules_view(request: Request, inertia: InertiaDep) -> InertiaResponse: +@router.get("/", response_model=None) +async def modules_view( + request: Request, + inertia: InertiaDep, + service: SettingService = Depends(get_setting_service), +) -> InertiaResponse: """Read-only view of every module's pydantic ``BaseSettings`` instance. Auto-discovered from ``app.state.sm.modules``; secrets are masked server-side. + Each field also reports where its live value came from — a stored override, + an ``SM_*`` env var, or the field default — so a setting that "isn't taking + effect" explains itself. """ - views = collect_module_settings(request.app) + overrides = await _overrides_by_package(request, service) + views = collect_module_settings(request.app, overrides) return await inertia.render( _PAGE_MODULES_EDIT, - {PROP_MODULES: serialize(views)}, + { + PROP_MODULES: serialize(views), + # Which packages can be connection-tested, so the page only offers + # the button where something is actually reachable. + "testable": _testable_packages(request), + }, ) + + +async def _overrides_by_package( + request: Request, service: SettingService +) -> dict[str, frozenset[str]]: + """Map package -> field names carrying a stored override.""" + from settings.store import SettingsStore + + store = SettingsStore(service) + packages = {v.package for v in collect_module_settings(request.app)} + return {pkg: frozenset(await store.get_overrides(pkg)) for pkg in packages} + + +def _testable_packages(request: Request) -> list[str]: + """Packages whose module registered at least one health check. + + "Test connection" is just that module's health checks run on demand — + reusing the registry means settings never learns what an SMTP or an S3 + connection is. + """ + registry = request.app.state.sm.health_registry + owners = {c.module for c in registry.all_checks if c.module} + return sorted( + { + _package_of_module(mod) + for mod in getattr(request.app.state.sm, "modules", ()) + if mod.meta.name in owners + } + ) + + +def _package_of_module(mod: object) -> str: + return type(mod).__module__.split(".", 1)[0] + + +@router.post("/test-connection/{package}", response_model=None) +async def test_connection(package: str, request: Request) -> dict: + """Run one module's health checks now and report each result. + + Returns 200 with per-check results even when a check fails: an admin + testing a connection expects to read the failure, not to get an error + status with the reason buried. + """ + modules = getattr(request.app.state.sm, "modules", ()) + owner = next((m for m in modules if _package_of_module(m) == package), None) + if owner is None: + raise HTTPException(status_code=404, detail=f"Unknown module package: {package}") + + checks = [c for c in request.app.state.sm.health_registry.all_checks if c.module == owner.meta.name] + if not checks: + raise HTTPException(status_code=404, detail=f"{owner.meta.name} has no connection to test") + + results = [] + for check in checks: + try: + outcome = await check.check() + results.append( + {"name": check.name, "status": outcome.status.value, "detail": outcome.detail or ""} + ) + except Exception as exc: + results.append({"name": check.name, "status": "unhealthy", "detail": str(exc)}) + return {"module": owner.meta.name, "checks": results} diff --git a/modules/settings/settings/locales/en.json b/modules/settings/settings/locales/en.json index 37f018de..648b593e 100644 --- a/modules/settings/settings/locales/en.json +++ b/modules/settings/settings/locales/en.json @@ -39,7 +39,8 @@ "value_label": "Value", "value_placeholder": "Enter a value", "description_label": "Description", - "description_placeholder": "Optional description" + "description_placeholder": "Optional description", + "key_unknown_warning": "No installed module declares this key. It will be stored, but nothing will read it unless a module looks it up." }, "create": { "title": "New Setting", @@ -66,6 +67,16 @@ "value": "Value", "default": "Default", "description": "Description" - } + }, + "test_connection": "Test connection", + "testing": "Testing…", + "env_var_hint": "Environment variable read when no stored override exists", + "source_db": "Stored override", + "source_db_hint": "A stored setting supplies this value", + "source_db_over_env": "Stored (shadows env)", + "source_db_shadows_env": "A stored setting overrides {env_var}; the environment value is ignored", + "source_env": "From environment", + "source_env_hint": "{env_var} is set in this deployment", + "source_default": "Default" } } diff --git a/modules/settings/settings/pages/Create.tsx b/modules/settings/settings/pages/Create.tsx index e4def08d..3e553961 100644 --- a/modules/settings/settings/pages/Create.tsx +++ b/modules/settings/settings/pages/Create.tsx @@ -15,13 +15,16 @@ import { import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; import type React from 'react'; +import { KeyField, type KnownKey } from './components/KeyField'; import ValueInput, { VALUE_TYPES, type ValueType } from './components/ValueInput'; import { ROUTES } from './routes'; const SCOPES = ['system', 'tenant', 'user'] as const; type Scope = (typeof SCOPES)[number]; -function Create() { +type Props = { known_keys?: KnownKey[] }; + +function Create({ known_keys }: Props) { const { t } = useT(); const { data, setData, post, processing, errors } = useForm({ scope: 'system' as Scope, @@ -83,19 +86,18 @@ function Create() { {errors.scope_id &&

{errors.scope_id}

}
-
- - setData('key', e.target.value)} - required - placeholder={t(keys.settings.form.key_placeholder)} - className="font-mono" - /> - {errors.key &&

{errors.key}

} -
+ { + setData((prev) => ({ + ...prev, + key, + ...(type ? { value_type: type as ValueType } : {}), + })); + }} + />
- +
+ {testable && } + +
{Object.entries(grouped).map(([group, fields]) => ( @@ -128,6 +137,7 @@ export function ModuleForm({ module: m }: Props) { Requires restart )} +
(null); + const [error, setError] = useState(null); + + async function run() { + setBusy(true); + setError(null); + setResults(null); + try { + const resp = await fetch(`/settings/test-connection/${pkg}`, { method: 'POST' }); + if (!resp.ok) throw new Error(resp.statusText); + const body = await resp.json(); + setResults(body.checks ?? []); + } catch (err) { + setError((err as Error).message); + } finally { + setBusy(false); + } + } + + return ( +
+ + + {error &&

{error}

} + + {results?.map((result) => { + const ok = result.status === 'healthy'; + return ( +

+ {ok ? : } + {/* The reason is the whole point: "connection refused" and + "authentication failed" need different fixes. */} + {result.detail || result.status} +

+ ); + })} +
+ ); +} diff --git a/modules/settings/settings/pages/routes.ts b/modules/settings/settings/pages/routes.ts index 18a9ba0e..1c76cb1e 100644 --- a/modules/settings/settings/pages/routes.ts +++ b/modules/settings/settings/pages/routes.ts @@ -1,7 +1,10 @@ export const ROUTES = { - browse: '/settings', - modules: '/settings/modules', + /** Per-module forms — the section root, and where "Settings" now lands. */ + modules: '/settings/', + /** Raw key/value store, demoted from the root. */ + browse: '/settings/store', create: '/settings/create', edit: (id: number) => `/settings/${id}/edit`, byId: (id: number) => `/settings/${id}`, + testConnection: (pkg: string) => `/settings/test-connection/${pkg}`, } as const; diff --git a/modules/settings/tests/test_settings_field_sources.py b/modules/settings/tests/test_settings_field_sources.py new file mode 100644 index 00000000..be1e9339 --- /dev/null +++ b/modules/settings/tests/test_settings_field_sources.py @@ -0,0 +1,101 @@ +"""Where a module setting's live value actually came from. + +The module-settings screen listed a value and its env var name but never said +which one was in force, so "I set SM_USERS_SMTP_HOST and nothing changed" +(because a stored override shadowed it) was invisible on the screen. +""" + +from __future__ import annotations + +import pytest +from settings._module_settings import ModuleSettingField + + +def _first_field(instance) -> str: + """First declared field name. Kept out of the async tests — a bare next() + raising StopIteration inside a coroutine surfaces as an unrelated + RuntimeError.""" + names = list(type(instance).model_fields) + assert names, f"{type(instance).__name__} declares no fields" + return names[0] + + +def _field(**overrides) -> ModuleSettingField: + base = { + "name": "smtp_host", + "env_var": "SM_USERS_SMTP_HOST", + "value": "mail.example.com", + "default": "localhost", + "description": "", + "is_secret": False, + "type": "string", + "requires_restart": False, + "group": None, + } + return ModuleSettingField(**{**base, **overrides}) + + +class TestFieldSource: + def test_plain_field_reports_default(self): + assert _field().source == "default" + + def test_env_var_present_reports_env(self): + assert _field(env_set=True).source == "env" + + def test_stored_override_reports_db(self): + assert _field(db_override=True).source == "db" + + def test_db_override_beats_env(self): + """Mirrors hydrate_settings: DB values are passed to the constructor, + so pydantic never consults the environment for that field.""" + assert _field(env_set=True, db_override=True).source == "db" + + +class TestModulesView: + async def test_fields_carry_their_source(self, authenticated_client): + resp = await authenticated_client.get("/settings/", follow_redirects=False) + assert resp.status_code == 200 + + def test_env_var_presence_is_detected(self, monkeypatch: pytest.MonkeyPatch): + """A deployment that sets the env var must not read as 'Default'.""" + from file_storage.settings import FileStorageSettings + from settings._module_settings import _field_view + + instance = FileStorageSettings() + name = _first_field(instance) + + assert _field_view(name, instance, "SM_FILE_STORAGE_").env_set is False + monkeypatch.setenv(f"SM_FILE_STORAGE_{name.upper()}", "x") + assert _field_view(name, instance, "SM_FILE_STORAGE_").env_set is True + + def test_overrides_mark_their_fields(self): + from file_storage.settings import FileStorageSettings + from settings._module_settings import _field_view + + instance = FileStorageSettings() + name = _first_field(instance) + view = _field_view(name, instance, "SM_FILE_STORAGE_", frozenset({name})) + assert view.db_override is True + assert view.source == "db" + + +class TestTestConnectionEndpoint: + async def test_unknown_package_is_a_404(self, authenticated_client): + resp = await authenticated_client.post("/settings/test-connection/nosuchmodule") + assert resp.status_code == 404 + + async def test_module_without_checks_is_a_404(self, authenticated_client): + """Only modules that registered a check can be tested.""" + resp = await authenticated_client.post("/settings/test-connection/settings") + assert resp.status_code == 404 + + async def test_failing_check_still_returns_200_with_the_reason(self, authenticated_client): + """An admin testing a connection needs to read the failure, not get an + error status with the reason buried.""" + resp = await authenticated_client.post("/settings/test-connection/file_storage") + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["checks"], body + for check in body["checks"]: + assert check["status"] in ("healthy", "degraded", "unhealthy") + assert "detail" in check diff --git a/modules/users/users/health.py b/modules/users/users/health.py new file mode 100644 index 00000000..acd806dc --- /dev/null +++ b/modules/users/users/health.py @@ -0,0 +1,49 @@ +"""Health check for the configured mailer. + +Doubles as the "Test connection" action on the module-settings screen: an +admin who has just typed SMTP credentials needs a way to find out they are +wrong that is cheaper than triggering a password reset and waiting. +""" + +from __future__ import annotations + +from fastapi import FastAPI +from simple_module_core.health import HealthCheckResult, HealthStatus + +CHECK_MAILER = "users.mailer" + + +def build_mailer_check(app: FastAPI): + """Return an async check closing over *app* so it re-reads live settings. + + Bound to the app rather than a mailer instance because settings are + hydrated from the DB and can change after boot — a check pinned to the + boot-time mailer would keep testing credentials the admin has replaced. + """ + + async def check() -> HealthCheckResult: + services = getattr(app.state, "users", None) + mailer = getattr(services, "mailer", None) + if mailer is None: + return HealthCheckResult( + status=HealthStatus.UNHEALTHY, detail="No mailer configured" + ) + + verify = getattr(mailer, "verify_connection", None) + if verify is None: + # The console mailer writes links to the log; there is nothing to + # reach, so it is healthy by construction rather than untested. + return HealthCheckResult( + status=HealthStatus.HEALTHY, + detail=f"{type(mailer).__name__} needs no connection", + ) + + try: + await verify() + except Exception as exc: + # The reason matters more than the traceback: "authentication + # failed" and "connection refused" call for different fixes. + return HealthCheckResult(status=HealthStatus.UNHEALTHY, detail=str(exc)) + return HealthCheckResult(status=HealthStatus.HEALTHY, detail="SMTP reachable") + + return check diff --git a/modules/users/users/mailer/smtp.py b/modules/users/users/mailer/smtp.py index a6a065e7..552cd8ce 100644 --- a/modules/users/users/mailer/smtp.py +++ b/modules/users/users/mailer/smtp.py @@ -71,6 +71,28 @@ async def send_invite(self, email: str, token: str, invited_by_name: str) -> Non body = template.render(link=link, invited_by_name=invited_by_name, app_name=app) await self._send(email, f"{invited_by_name} invited you to {app}", body) + async def verify_connection(self) -> None: + """Open an SMTP session and authenticate, then hang up. + + Deliberately stops short of sending anything: an admin checking their + mailer config should not put a stray message in someone's inbox. This + catches the failures that actually happen — wrong host or port, TLS + mismatch, bad credentials — and raises whatever aiosmtplib raises so + the caller can show the real reason. + """ + client = aiosmtplib.SMTP(hostname=self._host, port=self._port, use_tls=self._use_tls) + await client.connect() + try: + if self._username: + await client.login(self._username, self._password or "") + finally: + # noop() before quit keeps a server that dislikes an abrupt close + # from logging this probe as an error. + try: + await client.quit() + except Exception: + pass + async def _send(self, to: str, subject: str, body: str) -> None: message = EmailMessage() message["From"] = self._from diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 30a7e642..697306c8 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -204,6 +204,18 @@ def _app_name() -> str: return name or default_app_name() state.mailer = build_mailer(s, _app_name) + + # Registered here rather than in register_health_checks because the + # check needs the app to re-read DB-hydrated settings on every run. + # The owner is passed explicitly since the boot-time set_owner window + # has long closed by startup. + from simple_module_core.health import HealthCheck + + from users.health import CHECK_MAILER, build_mailer_check + + app.state.sm.health_registry.add( + HealthCheck(name=CHECK_MAILER, check=build_mailer_check(app), module=self.meta.name) + ) state.rate_limiter = LoginRateLimiter( max_failures=s.login_rate_limit_failures, window_seconds=s.login_rate_limit_window_seconds, diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index dc9b942d..9b41e162 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -313,6 +313,7 @@ export default { 'settings.form.description_placeholder': '', 'settings.form.key_label': '', 'settings.form.key_placeholder': '', + 'settings.form.key_unknown_warning': '', 'settings.form.scope_id_label': '', 'settings.form.scope_id_placeholder': '', 'settings.form.scope_label': '', @@ -324,15 +325,25 @@ export default { 'settings.modules.browse_link': '', 'settings.modules.description': '', 'settings.modules.empty_title': '', + 'settings.modules.env_var_hint': '', 'settings.modules.field_count_suffix': '', 'settings.modules.no_fields': '', 'settings.modules.search_placeholder': '', 'settings.modules.secret_badge': '', + 'settings.modules.source_db': '', + 'settings.modules.source_db_hint': '', + 'settings.modules.source_db_over_env': '', + 'settings.modules.source_db_shadows_env': '', + 'settings.modules.source_default': '', + 'settings.modules.source_env': '', + 'settings.modules.source_env_hint': '', 'settings.modules.table.default': '', 'settings.modules.table.description': '', 'settings.modules.table.env_var': '', 'settings.modules.table.field': '', 'settings.modules.table.value': '', + 'settings.modules.test_connection': '', + 'settings.modules.testing': '', 'settings.modules.title': '', 'settings.scopes.system': '', 'settings.scopes.tenant': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 56d45ed7..57acb5a4 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -420,6 +420,7 @@ export const keys = { description_placeholder: 'settings.form.description_placeholder', key_label: 'settings.form.key_label', key_placeholder: 'settings.form.key_placeholder', + key_unknown_warning: 'settings.form.key_unknown_warning', scope_id_label: 'settings.form.scope_id_label', scope_id_placeholder: 'settings.form.scope_id_placeholder', scope_label: 'settings.form.scope_label', @@ -433,10 +434,18 @@ export const keys = { browse_link: 'settings.modules.browse_link', description: 'settings.modules.description', empty_title: 'settings.modules.empty_title', + env_var_hint: 'settings.modules.env_var_hint', field_count_suffix: 'settings.modules.field_count_suffix', no_fields: 'settings.modules.no_fields', search_placeholder: 'settings.modules.search_placeholder', secret_badge: 'settings.modules.secret_badge', + source_db: 'settings.modules.source_db', + source_db_hint: 'settings.modules.source_db_hint', + source_db_over_env: 'settings.modules.source_db_over_env', + source_db_shadows_env: 'settings.modules.source_db_shadows_env', + source_default: 'settings.modules.source_default', + source_env: 'settings.modules.source_env', + source_env_hint: 'settings.modules.source_env_hint', table: { default: 'settings.modules.table.default', description: 'settings.modules.table.description', @@ -444,6 +453,8 @@ export const keys = { field: 'settings.modules.table.field', value: 'settings.modules.table.value', }, + test_connection: 'settings.modules.test_connection', + testing: 'settings.modules.testing', title: 'settings.modules.title', }, scopes: { From 33acb655dc0dc8c7691d49967ee3b85b66b0f0de Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 11 Aug 2026 15:08:27 +0200 Subject: [PATCH 07/17] feat(users): merged add-people flow, bulk invites, single dirty state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1j — create and invite were separate pages behind separate buttons, so an admin chose between them before seeing what either involved. They take nearly the same inputs and differ in exactly one respect (who sets the password), which makes it a mode switch, not a fork in the navigation. Both old URLs redirect into the merged form with their mode preselected. 1k — folded into that flow rather than into the standalone invite page it replaces. Addresses are pasted as a block (newlines, commas, semicolons) and reported per-address: one already-registered address in a list of twenty must not discard the other nineteen. Repeats are collapsed and addresses lowercased so a pasted column cannot mint two invites for one person. Capped at 100 per submit, so one request cannot mint unbounded live tokens. The copy-link panel appears only when the server says delivery did not happen — the console mailer writes invite URLs to stdout and nowhere else, so an admin could otherwise create an invite with no way to deliver it. A mailer that does not declare itself is assumed to deliver, so a third-party mailer never leaks tokens by omission. Delivery failures also fall back to a link rather than stranding a half-finished invite. 1f — the accept card asked for a password while naming neither the invitee nor the access granted, so a forwarded link was indistinguishable from the right one. It now shows both, and says so when an invite has already been used instead of presenting a form guaranteed to fail. The preview decodes the token read-only: UserManager.verify marks the account verified as a side effect, so routing the preview through it would spend the invite just by looking at the page. A test pins that. 1l — details and roles now share one dirty state and one Save, with an unsaved-changes marker and a navigation guard; only changed sections are sent, so saving a renamed user does not rewrite every role assignment's audit trail. Status changes stay immediate on purpose: disable/enable and mark-verified are actions, not edits, and putting an account lockout behind a Save button would be worse than the inconsistency it removes. Also splits the boot-time module registration loop into _registrations.py to stay under the 300-line cap. --- framework/core/simple_module_core/__init__.py | 2 +- .../simple_module_hosting/_registrations.py | 65 ++++ .../simple_module_hosting/app_builder.py | 44 +-- .../tests/test_error_page_shared_props.py | 4 +- modules/audit_log/audit_log/pages/Browse.tsx | 60 +--- .../audit_log/pages/components/EntryCells.tsx | 74 +++++ modules/dashboard/tests/test_dashboard.py | 8 +- modules/file_storage/file_storage/service.py | 4 +- modules/settings/settings/endpoints/views.py | 4 +- modules/users/tests/test_users_bulk_invite.py | 155 +++++++++ modules/users/tests/test_views_admin.py | 34 +- modules/users/users/admin/api.py | 5 + modules/users/users/admin/bulk_invite.py | 111 +++++++ modules/users/users/admin/views.py | 44 ++- .../users/users/auth_local/invite_preview.py | 64 ++++ modules/users/users/auth_local/views.py | 20 +- modules/users/users/contracts/schemas.py | 29 ++ modules/users/users/health.py | 4 +- modules/users/users/mailer/console.py | 9 + modules/users/users/mailer/smtp.py | 10 +- modules/users/users/pages/AcceptInvite.tsx | 40 ++- modules/users/users/pages/Users/AddPeople.tsx | 212 +++++++++++++ modules/users/users/pages/Users/Create.tsx | 175 ----------- modules/users/users/pages/Users/Edit.tsx | 297 +++++++++--------- modules/users/users/pages/Users/Index.tsx | 22 +- modules/users/users/pages/Users/Invite.tsx | 147 --------- .../Users/components/CreateUserFields.tsx | 78 +++++ .../pages/Users/components/DetailsCard.tsx | 54 +--- .../pages/Users/components/InviteFields.tsx | 46 +++ .../pages/Users/components/InviteResults.tsx | 107 +++++++ .../pages/Users/components/MetadataCard.tsx | 85 +++++ .../pages/Users/components/RolePicker.tsx | 42 +++ .../pages/Users/components/RolesCard.tsx | 37 +++ 33 files changed, 1428 insertions(+), 664 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/_registrations.py create mode 100644 modules/audit_log/audit_log/pages/components/EntryCells.tsx create mode 100644 modules/users/tests/test_users_bulk_invite.py create mode 100644 modules/users/users/admin/bulk_invite.py create mode 100644 modules/users/users/auth_local/invite_preview.py create mode 100644 modules/users/users/pages/Users/AddPeople.tsx delete mode 100644 modules/users/users/pages/Users/Create.tsx delete mode 100644 modules/users/users/pages/Users/Invite.tsx create mode 100644 modules/users/users/pages/Users/components/CreateUserFields.tsx create mode 100644 modules/users/users/pages/Users/components/InviteFields.tsx create mode 100644 modules/users/users/pages/Users/components/InviteResults.tsx create mode 100644 modules/users/users/pages/Users/components/MetadataCard.tsx create mode 100644 modules/users/users/pages/Users/components/RolePicker.tsx create mode 100644 modules/users/users/pages/Users/components/RolesCard.tsx diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index b1de5332..65ba4730 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -45,9 +45,9 @@ __all__ = [ "DEFAULT_AUTH_PROVIDER", "FRAMEWORK_API_VERSION", - "CircularDependencyError", "AuditLink", "AuditLinkRegistry", + "CircularDependencyError", "DesignPack", "DesignPackRegistry", "DiagnosticLevel", diff --git a/framework/hosting/simple_module_hosting/_registrations.py b/framework/hosting/simple_module_hosting/_registrations.py new file mode 100644 index 00000000..b1372e1c --- /dev/null +++ b/framework/hosting/simple_module_hosting/_registrations.py @@ -0,0 +1,65 @@ +"""Phase 5 of boot — every module's declarative registration hooks. + +Extracted from ``app_builder.py`` to keep that file readable; this is the one +place that knows the full set of hooks a module may implement, and the order +they run in. ``app_builder.create_app`` is the only intended caller. +""" + +from __future__ import annotations + +import inspect +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from fastapi import FastAPI + + +def run_module_registrations( + modules: list, + *, + app: FastAPI, + event_bus, + menu_registry, + perm_registry, + ff_registry, + health_registry, + public_route_registry, + design_pack_registry, + audit_link_registry, +) -> None: + """Invoke each module's registration hooks, in dependency order. + + Health checks are attributed to the module that registers them, so the + dashboard can report health per module rather than one global number. The + owner is cleared afterwards: anything registered later — a module's + ``on_startup``, say — belongs to no module in this loop, and inheriting + the last one's name would be a lie. + """ + for mod in modules: + mod.register_menu_items(menu_registry) + mod.register_permissions(perm_registry) + mod.register_feature_flags(ff_registry) + dispatch_event_handlers(mod, event_bus, app) + health_registry.set_owner(mod.meta.name) + mod.register_health_checks(health_registry) + mod.register_public_routes(public_route_registry) + mod.register_design_packs(design_pack_registry) + mod.register_audit_links(audit_link_registry) + + health_registry.set_owner("") + + +def dispatch_event_handlers(mod, event_bus, app: FastAPI) -> None: + """Call ``mod.register_event_handlers`` with or without ``app``. + + Back-compat shim for modules that still override the one-arg form + ``(self, bus)``; passing ``app=`` to those crashes. + """ + sig = inspect.signature(mod.register_event_handlers) + accepts_app = "app" in sig.parameters or any( + p.kind is inspect.Parameter.VAR_KEYWORD for p in sig.parameters.values() + ) + if accepts_app: + mod.register_event_handlers(event_bus, app=app) + else: + mod.register_event_handlers(event_bus) diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 916854ab..822a7b82 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -2,7 +2,6 @@ from __future__ import annotations -import inspect import logging import os from collections.abc import AsyncGenerator @@ -34,6 +33,7 @@ register_host_settings, wire_module_routes, ) +from simple_module_hosting._registrations import run_module_registrations from simple_module_hosting.health import router as health_router from simple_module_hosting.i18n_manifest import build_i18n_registry, emit_frontend_types from simple_module_hosting.migrations import check_migrations @@ -81,22 +81,6 @@ def _resolve_project_root() -> Path: _PROJECT_ROOT = _resolve_project_root() -def _register_event_handlers(mod, event_bus: EventBus, app: FastAPI) -> None: - """Dispatch to ``mod.register_event_handlers`` with or without ``app``. - - Back-compat shim for modules that still override the one-arg form - ``(self, bus)``; passing ``app=`` to those crashes. - """ - sig = inspect.signature(mod.register_event_handlers) - accepts_app = "app" in sig.parameters or any( - p.kind is inspect.Parameter.VAR_KEYWORD for p in sig.parameters.values() - ) - if accepts_app: - mod.register_event_handlers(event_bus, app=app) - else: - mod.register_event_handlers(event_bus) - - def create_app(settings: Settings | None = None) -> FastAPI: """Build and configure the full FastAPI application. @@ -221,20 +205,18 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: print_diagnostics(settings_diagnostics) # ── Phase 5: Module registrations ────────────────────── - for mod in modules: - mod.register_menu_items(menu_registry) - mod.register_permissions(perm_registry) - mod.register_feature_flags(ff_registry) - _register_event_handlers(mod, event_bus, app) - health_registry.set_owner(mod.meta.name) - mod.register_health_checks(health_registry) - mod.register_public_routes(public_route_registry) - mod.register_design_packs(design_pack_registry) - mod.register_audit_links(audit_link_registry) - - # Stop attributing to the last module in the loop — anything registered - # after this point belongs to no module in particular. - health_registry.set_owner("") + run_module_registrations( + modules, + app=app, + event_bus=event_bus, + menu_registry=menu_registry, + perm_registry=perm_registry, + ff_registry=ff_registry, + health_registry=health_registry, + public_route_registry=public_route_registry, + design_pack_registry=design_pack_registry, + audit_link_registry=audit_link_registry, + ) attach_public_routes(app, settings, public_route_registry) diff --git a/framework/hosting/tests/test_error_page_shared_props.py b/framework/hosting/tests/test_error_page_shared_props.py index a2812867..f4ce69e1 100644 --- a/framework/hosting/tests/test_error_page_shared_props.py +++ b/framework/hosting/tests/test_error_page_shared_props.py @@ -67,7 +67,9 @@ async def test_error_page_carries_correlation_id( """The page shows this id so a support report can be joined to the logs.""" resp = await authenticated_client.get(_MISSING_PATH) props = _inertia_page(resp.text)["props"] - assert props.get("correlation_id"), f"no correlation_id on error page; props={sorted(props)}" + assert props.get("correlation_id"), ( + f"no correlation_id on error page; props={sorted(props)}" + ) # Must be the same id the response header advertises, or quoting it # back would point support at a different request. assert props["correlation_id"] == resp.headers.get("x-correlation-id") diff --git a/modules/audit_log/audit_log/pages/Browse.tsx b/modules/audit_log/audit_log/pages/Browse.tsx index dd534a99..a0759c41 100644 --- a/modules/audit_log/audit_log/pages/Browse.tsx +++ b/modules/audit_log/audit_log/pages/Browse.tsx @@ -1,6 +1,5 @@ -import { Head, Link, router, usePage } from '@inertiajs/react'; +import { Head, router, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; -import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Button } from '@simple-module-py/ui/components/ui/button'; @@ -17,6 +16,7 @@ import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedL import { ScrollText } from 'lucide-react'; import type React from 'react'; import { useState } from 'react'; +import { ActorCell, EntityCell, type EntityRef } from './components/EntryCells'; import { ALL, FilterBar, type FilterState } from './components/FilterBar'; interface Change { @@ -25,12 +25,6 @@ interface Change { new?: unknown; } -interface EntityRef { - /** null when no module claims this table — the id renders unlinked. */ - url: string | null; - label: string; -} - interface AuditEntryRead { id: string; entity_type: string; @@ -109,56 +103,6 @@ function ChangesList({ entry }: { entry: AuditEntryRead }) { ); } -/** - * Entity kind and id. The id is a link when the owning module registered one, - * and always copyable — quoting an id into a ticket is the other thing people - * do with this column. - */ -function EntityCell({ entry }: { entry: AuditEntryRead }) { - const label = entry.entity?.label ?? entry.entity_type; - const url = entry.entity?.url ?? null; - const shortId = entry.entity_id.length > 12 ? `${entry.entity_id.slice(0, 8)}…` : entry.entity_id; - - return ( -
- {label} - {url ? ( - - {shortId} - - ) : ( - - )} -
- ); -} - -/** Who acted: display name where the account still exists, raw id otherwise. */ -function ActorCell({ entry }: { entry: AuditEntryRead }) { - const { t } = useT(); - if (!entry.user_id) return <>{t(keys.audit_log.changes.system_user)}; - if (entry.actor) { - return ( - - {entry.actor} - - ); - } - // The account is gone. The id is still the truthful record of who acted, - // so show it rather than pretending the action had no author. - return ( - - ); -} - function Browse() { const { items, total, page, page_size, entity_types, filters } = usePage<{ props: Props }>() .props as unknown as Props; diff --git a/modules/audit_log/audit_log/pages/components/EntryCells.tsx b/modules/audit_log/audit_log/pages/components/EntryCells.tsx new file mode 100644 index 00000000..7ec8ebfb --- /dev/null +++ b/modules/audit_log/audit_log/pages/components/EntryCells.tsx @@ -0,0 +1,74 @@ +import { Link } from '@inertiajs/react'; +import { keys, useT } from '@simple-module-py/i18n'; +import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; + +export interface EntityRef { + /** null when no module claims this table — the id renders unlinked. */ + url: string | null; + label: string; +} + +export interface AuditEntryRef { + entity_type: string; + entity_id: string; + user_id: string | null; + /** Display name resolved from user_id, or null for deleted/system actors. */ + actor: string | null; + entity: EntityRef; +} + +const SHORT_ID_LENGTH = 8; +const SHORTEN_ABOVE = 12; + +function short(id: string): string { + return id.length > SHORTEN_ABOVE ? `${id.slice(0, SHORT_ID_LENGTH)}…` : id; +} + +/** + * Entity kind and id. The id is a link when the owning module registered one, + * and always copyable — quoting an id into a ticket is the other thing people + * do with this column. + */ +export function EntityCell({ entry }: { entry: AuditEntryRef }) { + const label = entry.entity?.label ?? entry.entity_type; + const url = entry.entity?.url ?? null; + + return ( +
+ {label} + {url ? ( + + {short(entry.entity_id)} + + ) : ( + + )} +
+ ); +} + +/** Who acted: display name where the account still exists, raw id otherwise. */ +export function ActorCell({ entry }: { entry: AuditEntryRef }) { + const { t } = useT(); + if (!entry.user_id) return <>{t(keys.audit_log.changes.system_user)}; + if (entry.actor) { + return ( + + {entry.actor} + + ); + } + // The account is gone. The id is still the truthful record of who acted, + // so show it rather than pretending the action had no author. + return ( + + ); +} diff --git a/modules/dashboard/tests/test_dashboard.py b/modules/dashboard/tests/test_dashboard.py index 536bf9d0..21c5d63d 100644 --- a/modules/dashboard/tests/test_dashboard.py +++ b/modules/dashboard/tests/test_dashboard.py @@ -99,7 +99,9 @@ async def test_stats_requires_authentication(self, client: httpx.AsyncClient): resp = await client.get(_STATS_URL, follow_redirects=False) assert resp.status_code in (302, 401, 403) - async def test_module_entries_carry_a_link_target(self, authenticated_client: httpx.AsyncClient): + async def test_module_entries_carry_a_link_target( + self, authenticated_client: httpx.AsyncClient + ): """Tiles were inert; each one now needs its module's own screen.""" resp = await authenticated_client.get(_STATS_URL) modules = {m["name"]: m for m in resp.json()["system_info"]["modules"]} @@ -113,9 +115,7 @@ async def test_view_less_modules_get_an_empty_url( for mod in resp.json()["system_info"]["modules"]: assert mod["url"] == "" or mod["url"].startswith("/"), mod - async def test_every_module_entry_reports_health( - self, authenticated_client: httpx.AsyncClient - ): + async def test_every_module_entry_reports_health(self, authenticated_client: httpx.AsyncClient): resp = await authenticated_client.get(_STATS_URL) for mod in resp.json()["system_info"]["modules"]: assert mod["health"] in ("", "healthy", "degraded", "unhealthy"), mod diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index 3165ef49..ebb95028 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -202,9 +202,7 @@ async def content_type_facets(self, *, created_by: str | None = None) -> list[di showing are the ones actually in the bucket. """ query = select(StoredFile.content_type, func.count().label("n")) - for clause in self._filter_clauses( - created_by=created_by, search=None, content_type=None - ): + for clause in self._filter_clauses(created_by=created_by, search=None, content_type=None): query = query.where(clause) query = query.group_by(StoredFile.content_type).order_by(StoredFile.content_type) diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 7488ee05..8c09767d 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -224,7 +224,9 @@ async def test_connection(package: str, request: Request) -> dict: if owner is None: raise HTTPException(status_code=404, detail=f"Unknown module package: {package}") - checks = [c for c in request.app.state.sm.health_registry.all_checks if c.module == owner.meta.name] + checks = [ + c for c in request.app.state.sm.health_registry.all_checks if c.module == owner.meta.name + ] if not checks: raise HTTPException(status_code=404, detail=f"{owner.meta.name} has no connection to test") diff --git a/modules/users/tests/test_users_bulk_invite.py b/modules/users/tests/test_users_bulk_invite.py new file mode 100644 index 00000000..45c30efc --- /dev/null +++ b/modules/users/tests/test_users_bulk_invite.py @@ -0,0 +1,155 @@ +"""Bulk invite — many addresses per submit, with per-address outcomes. + +The invite form took one address at a time, so onboarding a team meant +repeating the form once per person. Partial success is the normal case here: +one already-registered address must not discard the rest. +""" + +from __future__ import annotations + +import httpx +import pytest + +_URL = "/api/users/admin/invite/bulk" + + +class TestBulkInvite: + async def test_invites_every_address(self, authenticated_client: httpx.AsyncClient): + resp = await authenticated_client.post( + _URL, + json={"emails": ["a@example.com", "b@example.com"], "role_names": []}, + ) + assert resp.status_code == 200, resp.text + results = resp.json()["results"] + assert [r["email"] for r in results] == ["a@example.com", "b@example.com"] + + async def test_duplicate_addresses_are_invited_once( + self, authenticated_client: httpx.AsyncClient + ): + """Pasting a list with a repeat should not mint two invites for it.""" + resp = await authenticated_client.post( + _URL, + json={"emails": ["dup@example.com", "DUP@example.com"], "role_names": []}, + ) + assert len(resp.json()["results"]) == 1 + + async def test_addresses_are_normalised_to_lowercase( + self, authenticated_client: httpx.AsyncClient + ): + resp = await authenticated_client.post( + _URL, json={"emails": ["Mixed@Example.com"], "role_names": []} + ) + assert resp.json()["results"][0]["email"] == "mixed@example.com" + + async def test_one_failure_does_not_discard_the_others( + self, authenticated_client: httpx.AsyncClient + ): + """A duplicate in a pasted list of twenty must not lose the other 19.""" + await authenticated_client.post( + _URL, json={"emails": ["taken@example.com"], "role_names": []} + ) + resp = await authenticated_client.post( + _URL, + json={"emails": ["taken@example.com", "fresh@example.com"], "role_names": []}, + ) + assert resp.status_code == 200, resp.text + by_email = {r["email"]: r for r in resp.json()["results"]} + assert by_email["taken@example.com"]["status"] == "failed" + assert by_email["fresh@example.com"]["status"] in ("sent", "link") + + async def test_console_mailer_hands_back_a_copyable_link( + self, authenticated_client: httpx.AsyncClient + ): + """The test app uses the console mailer, which delivers nothing — the + admin needs the link or the invite is undeliverable.""" + resp = await authenticated_client.post( + _URL, json={"emails": ["linkme@example.com"], "role_names": []} + ) + result = resp.json()["results"][0] + assert result["status"] == "link" + assert "/users/invite/accept?token=" in result["link"] + + async def test_roles_apply_to_every_address(self, authenticated_client: httpx.AsyncClient): + resp = await authenticated_client.post( + _URL, + json={"emails": ["r1@example.com", "r2@example.com"], "role_names": ["user"]}, + ) + assert resp.status_code == 200, resp.text + assert all(r["status"] in ("sent", "link") for r in resp.json()["results"]) + + async def test_empty_list_is_accepted_and_does_nothing( + self, authenticated_client: httpx.AsyncClient + ): + resp = await authenticated_client.post(_URL, json={"emails": [], "role_names": []}) + assert resp.status_code == 200 + assert resp.json()["results"] == [] + + async def test_address_count_is_capped(self, authenticated_client: httpx.AsyncClient): + """One submit must not be able to mint unbounded live invite tokens.""" + from users.admin.bulk_invite import MAX_ADDRESSES + + emails = [f"bulk{i}@example.com" for i in range(MAX_ADDRESSES + 5)] + resp = await authenticated_client.post(_URL, json={"emails": emails, "role_names": []}) + assert len(resp.json()["results"]) == MAX_ADDRESSES + + async def test_requires_authentication(self, client: httpx.AsyncClient): + resp = await client.post( + _URL, json={"emails": ["x@example.com"], "role_names": []}, follow_redirects=False + ) + assert resp.status_code in (302, 401, 403) + + +class TestInvitePreview: + async def test_accept_page_names_the_invitee( + self, authenticated_client: httpx.AsyncClient, client: httpx.AsyncClient + ): + """The card asked for a password while identifying nobody.""" + created = await authenticated_client.post( + _URL, json={"emails": ["preview@example.com"], "role_names": []} + ) + link = created.json()["results"][0]["link"] + token = link.split("token=")[1] + + resp = await client.get( + f"/users/invite/accept?token={token}", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert resp.status_code == 200, resp.text + invite = resp.json()["props"]["invite"] + assert invite["email"] == "preview@example.com" + assert invite["already_accepted"] is False + + @pytest.mark.parametrize("token", ["", "not-a-jwt", "a.b.c"]) + async def test_unreadable_tokens_yield_no_preview(self, client: httpx.AsyncClient, token: str): + """Expired, tampered and absent all look the same here on purpose — + the reason belongs to the accept attempt, which validates properly.""" + resp = await client.get( + f"/users/invite/accept?token={token}", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert resp.status_code == 200 + assert resp.json()["props"]["invite"] is None + + async def test_preview_does_not_consume_the_invite( + self, authenticated_client: httpx.AsyncClient, client: httpx.AsyncClient + ): + """Viewing the page must leave the token usable — UserManager.verify + marks the account verified as a side effect, so the preview cannot + route through it.""" + created = await authenticated_client.post( + _URL, json={"emails": ["unspent@example.com"], "role_names": []} + ) + token = created.json()["results"][0]["link"].split("token=")[1] + + for _ in range(2): + resp = await client.get( + f"/users/invite/accept?token={token}", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert resp.json()["props"]["invite"]["already_accepted"] is False + + accepted = await client.post( + "/api/users/auth/accept-invite", + json={"token": token, "password": "a-good-password-123"}, + ) + assert accepted.status_code in (200, 204), accepted.text diff --git a/modules/users/tests/test_views_admin.py b/modules/users/tests/test_views_admin.py index 82391419..2210129d 100644 --- a/modules/users/tests/test_views_admin.py +++ b/modules/users/tests/test_views_admin.py @@ -129,23 +129,43 @@ async def test_flag_false_when_not_installed(self, admin_client, users_app, user # --------------------------------------------------------------------------- -# Admin create page +# Admin add-people page (create + invite merged behind a mode switch) # --------------------------------------------------------------------------- -class TestAdminCreatePage: +class TestAdminAddPeoplePage: @pytest.mark.anyio - async def test_create_page_renders_with_roles(self, admin_client): + async def test_add_page_renders_with_roles(self, admin_client): resp = await admin_client.get( - "/users/admin/create", + "/users/admin/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 data = resp.json() - assert data["component"] == "Users/Users/Create" + assert data["component"] == "Users/Users/AddPeople" assert "roles" in data["props"] @pytest.mark.anyio - async def test_create_page_requires_auth(self, anon_client): - resp = await anon_client.get("/users/admin/create", follow_redirects=False) + async def test_add_page_reports_whether_mail_can_be_delivered(self, admin_client): + """Drives the copy-link panel — the page has to know before submitting.""" + resp = await admin_client.get( + "/users/admin/add", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert "mailer_delivers" in resp.json()["props"] + + @pytest.mark.anyio + async def test_add_page_requires_auth(self, anon_client): + resp = await anon_client.get("/users/admin/add", follow_redirects=False) assert resp.status_code == 302 + + @pytest.mark.anyio + @pytest.mark.parametrize( + ("old_path", "mode"), + [("/users/admin/create", "create"), ("/users/admin/invite", "invite")], + ) + async def test_old_urls_redirect_into_the_right_mode(self, admin_client, old_path, mode): + """Existing links must land on the merged form with their mode preselected.""" + resp = await admin_client.get(old_path, follow_redirects=False) + assert resp.status_code == 307 + assert resp.headers["location"] == f"/users/admin/add?mode={mode}" diff --git a/modules/users/users/admin/api.py b/modules/users/users/admin/api.py index 2a8e2b14..bb68f1af 100644 --- a/modules/users/users/admin/api.py +++ b/modules/users/users/admin/api.py @@ -10,6 +10,7 @@ from simple_module_core.events import EventBus from simple_module_hosting.permissions import RequiresPermission +from users.admin.bulk_invite import bulk_router from users.admin.service import UserService from users.constants import PERM_USERS_MANAGE, sanitize_list_filters from users.contracts.events import ( @@ -40,6 +41,10 @@ tags=["users-admin"], ) +# Bulk invite lives in its own module (this file is near the 300-line cap) but +# mounts here so it inherits the users.manage guard above. +admin_router.include_router(bulk_router) + @admin_router.get("", response_model=list[UserListItem]) async def admin_list_users( diff --git a/modules/users/users/admin/bulk_invite.py b/modules/users/users/admin/bulk_invite.py new file mode 100644 index 00000000..f23d363c --- /dev/null +++ b/modules/users/users/admin/bulk_invite.py @@ -0,0 +1,111 @@ +"""Bulk invite — one submit, many addresses. + +The invite form took a single address, so onboarding a team meant repeating +the same form once per person. This accepts a pasted list and reports each +address separately: one already-registered address in a list of twenty must +not discard the other nineteen. +""" + +from __future__ import annotations + +import logging + +from fastapi import APIRouter, Depends, Request +from simple_module_core.events import EventBus + +from users.admin.service import UserService +from users.contracts.events import UserInvited +from users.contracts.schemas import BulkInviteResponse, BulkInviteResult, UserBulkInvite +from users.deps import get_event_bus, get_mailer, get_user_service + +logger = logging.getLogger(__name__) + +bulk_router = APIRouter() + +STATUS_SENT = "sent" +STATUS_LINK = "link" +STATUS_FAILED = "failed" + +MAX_ADDRESSES = 100 +"""Enough for a team, small enough that one submit cannot mint an unbounded +number of live invite tokens.""" + + +def _invite_link(request: Request, token: str) -> str: + base = str(request.base_url).rstrip("/") + return f"{base}/users/invite/accept?token={token}" + + +@bulk_router.post("/invite/bulk", response_model=BulkInviteResponse) +async def admin_bulk_invite( + data: UserBulkInvite, + request: Request, + bus: EventBus = Depends(get_event_bus), + service: UserService = Depends(get_user_service), + mailer=Depends(get_mailer), +) -> BulkInviteResponse: + """Invite every address in *data*, all sharing the same roles.""" + invited_by = getattr(request.state, "user", None) + invited_by_name = invited_by.name if invited_by else "Administrator" + + # Absent attribute means "assume it delivers" — a third-party mailer must + # never leak invite tokens into the response just by not declaring itself. + delivers = getattr(mailer, "delivers_email", True) + + # Preserve submit order but drop repeats: pasting a list with the same + # address twice should not create two invites for it. + seen: set[str] = set() + ordered: list[str] = [] + for raw in data.emails[:MAX_ADDRESSES]: + email = str(raw).strip().lower() + if email and email not in seen: + seen.add(email) + ordered.append(email) + + results: list[BulkInviteResult] = [] + for email in ordered: + try: + user, token = await service.invite(email, None, data.role_names, invited_by=invited_by) + except Exception as exc: + # Already-registered is the common case and reads fine as-is; + # anything else is logged so the admin's summary stays short. + logger.info("bulk invite failed for %s: %s", email, exc) + results.append(BulkInviteResult(email=email, status=STATUS_FAILED, detail=str(exc))) + continue + + if delivers: + try: + await mailer.send_invite(user.email, token, invited_by_name) + except Exception as exc: + # The account exists and the token is valid — the delivery + # failed. Handing back the link turns a dead end into a + # copy-paste, rather than stranding a half-finished invite. + logger.warning("invite mail failed for %s: %s", email, exc) + results.append( + BulkInviteResult( + email=email, + status=STATUS_LINK, + detail=str(exc), + link=_invite_link(request, token), + ) + ) + else: + results.append(BulkInviteResult(email=email, status=STATUS_SENT)) + else: + results.append( + BulkInviteResult( + email=email, + status=STATUS_LINK, + link=_invite_link(request, token), + ) + ) + + await bus.publish( + UserInvited( + user_id=user.id, + email=user.email, + invited_by=(str(invited_by.id) if invited_by else None), + ) + ) + + return BulkInviteResponse(results=results) diff --git a/modules/users/users/admin/views.py b/modules/users/users/admin/views.py index 5b070c5d..bd6e0c16 100644 --- a/modules/users/users/admin/views.py +++ b/modules/users/users/admin/views.py @@ -8,6 +8,7 @@ from inertia import InertiaResponse from simple_module_hosting.inertia_deps import InertiaDep from simple_module_hosting.permissions import RequiresPermission +from starlette.responses import RedirectResponse from users.admin.service import UserService from users.constants import PERM_USERS_MANAGE, sanitize_list_filters @@ -18,8 +19,7 @@ router = APIRouter() _PAGE_ADMIN_INDEX = "Users/Users/Index" -_PAGE_ADMIN_INVITE = "Users/Users/Invite" -_PAGE_ADMIN_CREATE = "Users/Users/Create" +_PAGE_ADMIN_ADD = "Users/Users/AddPeople" _PAGE_ADMIN_EDIT = "Users/Users/Edit" @@ -81,37 +81,51 @@ async def admin_index( @router.get( - "/admin/invite", + "/admin/add", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) -async def admin_invite_page( +async def admin_add_people_page( request: Request, inertia: InertiaDep, ) -> InertiaResponse: + """One screen for both ways of adding people, chosen by a mode switch. + + Create and invite were separate pages reached from separate buttons, which + made an admin decide between them before seeing what either involved. They + take almost the same inputs and differ in one respect — who sets the + password — so the choice belongs inside the form. + """ + mailer = getattr(getattr(request.app.state, "users", None), "mailer", None) return await inertia.render( - _PAGE_ADMIN_INVITE, + _PAGE_ADMIN_ADD, { "roles": await _roles_payload(request.app), + # Drives the copy-link panel: when nothing can be delivered, the + # invite mode has to hand the link back instead. + "mailer_delivers": bool(getattr(mailer, "delivers_email", True)), }, ) +@router.get( + "/admin/invite", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], +) +async def admin_invite_redirect() -> RedirectResponse: + """Old invite URL — the flow merged into /users/admin/add.""" + return RedirectResponse("/users/admin/add?mode=invite", status_code=307) + + @router.get( "/admin/create", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) -async def admin_create_page( - request: Request, - inertia: InertiaDep, -) -> InertiaResponse: - return await inertia.render( - _PAGE_ADMIN_CREATE, - { - "roles": await _roles_payload(request.app), - }, - ) +async def admin_create_redirect() -> RedirectResponse: + """Old create URL — the flow merged into /users/admin/add.""" + return RedirectResponse("/users/admin/add?mode=create", status_code=307) @router.get( diff --git a/modules/users/users/auth_local/invite_preview.py b/modules/users/users/auth_local/invite_preview.py new file mode 100644 index 00000000..3e916d8f --- /dev/null +++ b/modules/users/users/auth_local/invite_preview.py @@ -0,0 +1,64 @@ +"""Read an invite token without spending it. + +The accept-invite card asked for a password while showing neither who the +invite was for nor what access it grants. Someone forwarded a link, or holding +two invites to different deployments, had no way to tell them apart — and no +way to notice an invite addressed to the wrong person before accepting it. + +``UserManager.verify`` cannot answer this: it marks the account verified as a +side effect, so calling it to peek would consume the invite. The verification +token is a JWT carrying ``sub`` and ``email``, so decoding it read-only gives +the same facts with no side effects. +""" + +from __future__ import annotations + +import logging +from typing import Any + +import jwt +from fastapi_users.jwt import decode_jwt + +logger = logging.getLogger(__name__) + + +async def preview_invite(token: str, user_manager: Any) -> dict[str, Any] | None: + """Return ``{"email", "roles"}`` for *token*, or ``None`` if unreadable. + + ``None`` covers expired, tampered, and wrong-audience tokens alike. The + page deliberately does not distinguish them: the reason belongs to the + accept attempt, which validates properly. + """ + if not token: + return None + + try: + data = decode_jwt( + token, + user_manager.verification_token_secret, + [user_manager.verification_token_audience], + ) + except jwt.PyJWTError: + return None + + email = data.get("email") + if not email: + return None + + roles: list[str] = [] + try: + user = await user_manager.get_by_email(email) + except Exception: + # The token decoded but the account is gone. Showing the address it + # was issued for is still more useful than showing nothing; accepting + # will fail with a proper message. + return {"email": email, "roles": roles, "already_accepted": False} + + roles = sorted(role.name for role in getattr(user, "roles", []) or []) + return { + "email": email, + "roles": roles, + # An invite that has already been used should say so, rather than + # presenting a password form that is guaranteed to fail. + "already_accepted": bool(getattr(user, "is_verified", False)), + } diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index eafaec1f..cf694526 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -2,12 +2,14 @@ from __future__ import annotations -from fastapi import APIRouter, HTTPException, Request +from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse from simple_module_hosting.inertia_deps import InertiaDep from starlette.responses import RedirectResponse +from users.auth_local.invite_preview import preview_invite from users.bootstrap import resolve_bootstrap_credentials +from users.manager import UserManager, get_user_manager router = APIRouter() @@ -95,8 +97,20 @@ async def verify_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: @router.get("/invite/accept", response_model=None) -async def accept_invite_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: - return await inertia.render(_PAGE_ACCEPT_INVITE, {"token": token}) +async def accept_invite_page( + inertia: InertiaDep, + user_manager: UserManager = Depends(get_user_manager), + token: str = "", +) -> InertiaResponse: + """Show who the invite is for and what it grants, before asking for a password.""" + invite = await preview_invite(token, user_manager) + return await inertia.render( + _PAGE_ACCEPT_INVITE, + { + "token": token, + "invite": invite, + }, + ) @router.get("/me", response_model=None) diff --git a/modules/users/users/contracts/schemas.py b/modules/users/users/contracts/schemas.py index 31e9d27c..ca08f449 100644 --- a/modules/users/users/contracts/schemas.py +++ b/modules/users/users/contracts/schemas.py @@ -57,6 +57,35 @@ class UserInvite(SQLModel): role_names: list[str] = [] +class UserBulkInvite(SQLModel): + """Invite several addresses in one submit, all sharing the same roles.""" + + emails: list[EmailStr] + role_names: list[str] = [] + + +class BulkInviteResult(SQLModel): + """Outcome for a single address in a bulk invite. + + Per-address rather than all-or-nothing: one already-registered address in + a pasted list of twenty should not discard the other nineteen. + """ + + email: str + status: str + """``"sent"`` — mail dispatched. ``"link"`` — created, but the configured + mailer cannot deliver, so ``link`` carries the URL. ``"failed"`` — see + ``detail``.""" + detail: str = "" + link: str | None = None + """One-time accept URL. Populated only when the mailer cannot deliver; + otherwise the token stays out of the response entirely.""" + + +class BulkInviteResponse(SQLModel): + results: list[BulkInviteResult] + + class UserAdminCreate(SQLModel): email: EmailStr password: str diff --git a/modules/users/users/health.py b/modules/users/users/health.py index acd806dc..7af1f420 100644 --- a/modules/users/users/health.py +++ b/modules/users/users/health.py @@ -25,9 +25,7 @@ async def check() -> HealthCheckResult: services = getattr(app.state, "users", None) mailer = getattr(services, "mailer", None) if mailer is None: - return HealthCheckResult( - status=HealthStatus.UNHEALTHY, detail="No mailer configured" - ) + return HealthCheckResult(status=HealthStatus.UNHEALTHY, detail="No mailer configured") verify = getattr(mailer, "verify_connection", None) if verify is None: diff --git a/modules/users/users/mailer/console.py b/modules/users/users/mailer/console.py index b5a434fc..65b32d3b 100644 --- a/modules/users/users/mailer/console.py +++ b/modules/users/users/mailer/console.py @@ -12,6 +12,15 @@ class ConsoleMailer: + delivers_email = False + """Nothing leaves the process — links only reach the log. + + Callers that need the recipient to actually receive something (the bulk + invite screen) read this to decide whether to surface the one-time link + in the UI instead. Absence of the attribute means "assume it delivers", + so a third-party mailer never leaks tokens by omission. + """ + def __init__(self, base_url: str, app_name_provider: AppNameProvider | None = None) -> None: self._base = base_url.rstrip("/") from users.mailer import default_app_name diff --git a/modules/users/users/mailer/smtp.py b/modules/users/users/mailer/smtp.py index 552cd8ce..b6121617 100644 --- a/modules/users/users/mailer/smtp.py +++ b/modules/users/users/mailer/smtp.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import importlib.resources from email.message import EmailMessage from typing import TYPE_CHECKING @@ -86,12 +87,11 @@ async def verify_connection(self) -> None: if self._username: await client.login(self._username, self._password or "") finally: - # noop() before quit keeps a server that dislikes an abrupt close - # from logging this probe as an error. - try: + # A failure hanging up says nothing about whether the credentials + # work, which is the only question being asked — so it must not + # mask the login error this block is unwinding. + with contextlib.suppress(Exception): await client.quit() - except Exception: - pass async def _send(self, to: str, subject: str, body: str) -> None: message = EmailMessage() diff --git a/modules/users/users/pages/AcceptInvite.tsx b/modules/users/users/pages/AcceptInvite.tsx index aa5f928a..e2e2e18b 100644 --- a/modules/users/users/pages/AcceptInvite.tsx +++ b/modules/users/users/pages/AcceptInvite.tsx @@ -6,12 +6,20 @@ import { AuthCardShell } from '@simple-module-py/ui/layouts/AuthCardShell'; import { CheckCircle2 } from 'lucide-react'; import { useState } from 'react'; +interface InvitePreview { + email: string; + roles: string[]; + already_accepted: boolean; +} + interface Props { token: string; + /** null when the token cannot be read — expired, tampered, or absent. */ + invite: InvitePreview | null; } function AcceptInvite() { - const { token: initialToken } = usePage<{ props: Props }>().props as unknown as Props; + const { token: initialToken, invite } = usePage<{ props: Props }>().props as unknown as Props; const urlToken = typeof window !== 'undefined' ? (new URLSearchParams(window.location.search).get('token') ?? '') @@ -56,13 +64,39 @@ function AcceptInvite() { return ( + {/* Who the invite is for, and what it grants. Without this the card asks + for a password while identifying neither — a forwarded link, or an + invite addressed to the wrong person, is indistinguishable from the + right one. */}
diff --git a/modules/users/users/pages/Users/Index.tsx b/modules/users/users/pages/Users/Index.tsx index dc30b4c8..9a9410af 100644 --- a/modules/users/users/pages/Users/Index.tsx +++ b/modules/users/users/pages/Users/Index.tsx @@ -128,20 +128,14 @@ function Index() { title="Users" description="People with access to this workspace. Invites use the configured mailer." actions={ -
- - -
+ // One entry point: invite-vs-create is a choice inside the form, not + // a choice between two buttons made before seeing either. + } >
diff --git a/modules/users/users/pages/Users/Invite.tsx b/modules/users/users/pages/Users/Invite.tsx deleted file mode 100644 index f0caf4a4..00000000 --- a/modules/users/users/pages/Users/Invite.tsx +++ /dev/null @@ -1,147 +0,0 @@ -import { Link, router, usePage } from '@inertiajs/react'; -import { PageShell } from '@simple-module-py/ui/components/PageShell'; -import { Button } from '@simple-module-py/ui/components/ui/button'; -import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; -import { Input } from '@simple-module-py/ui/components/ui/input'; -import { Label } from '@simple-module-py/ui/components/ui/label'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; -import { Mail, Send } from 'lucide-react'; -import { useState } from 'react'; -import { toast } from 'sonner'; - -interface Role { - id: string; - name: string; -} - -interface Props { - roles: Role[]; -} - -function Invite() { - const { roles } = usePage<{ props: Props }>().props as unknown as Props; - - const [email, setEmail] = useState(''); - const [fullName, setFullName] = useState(''); - const [selectedRoles, setSelectedRoles] = useState([]); - const [error, setError] = useState(null); - const [loading, setLoading] = useState(false); - - const toggleRole = (roleName: string) => { - setSelectedRoles((prev) => - prev.includes(roleName) ? prev.filter((r) => r !== roleName) : [...prev, roleName], - ); - }; - - const handleSubmit = (e: React.FormEvent) => { - e.preventDefault(); - setError(null); - setLoading(true); - fetch('/api/users/admin/invite', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ email, full_name: fullName || null, role_names: selectedRoles }), - }) - .then(async (res) => { - if (res.ok) { - toast.success('Invite sent'); - router.visit('/users/admin'); - } else { - const data = await res.json().catch(() => ({})); - setError(typeof data?.detail === 'string' ? data.detail : 'Failed to send invite'); - } - }) - .catch(() => setError('An error occurred. Please try again.')) - .finally(() => setLoading(false)); - }; - - return ( - - Back to Users - - } - > - - -
-
- -
- - setEmail(e.target.value)} - placeholder="teammate@example.com" - required - autoComplete="off" - className="pl-9" - /> -
-
- -
- - setFullName(e.target.value)} - placeholder="Jane Doe" - /> -
- - {roles.length > 0 && ( -
- -
- {roles.map((role) => { - const active = selectedRoles.includes(role.name); - return ( - - ); - })} -
-
- )} - - {error &&

{error}

} - -
- - -
-
- - - - ); -} - -Invite.layout = (page: React.ReactNode) => {page}; -export default Invite; diff --git a/modules/users/users/pages/Users/components/CreateUserFields.tsx b/modules/users/users/pages/Users/components/CreateUserFields.tsx new file mode 100644 index 00000000..dbef6310 --- /dev/null +++ b/modules/users/users/pages/Users/components/CreateUserFields.tsx @@ -0,0 +1,78 @@ +import { Input } from '@simple-module-py/ui/components/ui/input'; +import { Label } from '@simple-module-py/ui/components/ui/label'; +import { Lock, Mail } from 'lucide-react'; + +interface Props { + email: string; + fullName: string; + password: string; + onEmailChange: (value: string) => void; + onFullNameChange: (value: string) => void; + onPasswordChange: (value: string) => void; +} + +export function CreateUserFields({ + email, + fullName, + password, + onEmailChange, + onFullNameChange, + onPasswordChange, +}: Props) { + return ( + <> +
+ +
+ + onEmailChange(e.target.value)} + placeholder="teammate@example.com" + required + autoComplete="off" + className="pl-9" + /> +
+
+ +
+ + onFullNameChange(e.target.value)} + placeholder="Jane Doe" + /> +
+ +
+ +
+ + onPasswordChange(e.target.value)} + required + autoComplete="new-password" + className="pl-9" + /> +
+

+ The account is active and verified immediately — share the password securely. +

+
+ + ); +} diff --git a/modules/users/users/pages/Users/components/DetailsCard.tsx b/modules/users/users/pages/Users/components/DetailsCard.tsx index bd1de671..6fa82d94 100644 --- a/modules/users/users/pages/Users/components/DetailsCard.tsx +++ b/modules/users/users/pages/Users/components/DetailsCard.tsx @@ -1,53 +1,25 @@ import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; -import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; import { Label } from '@simple-module-py/ui/components/ui/label'; -import { useState } from 'react'; -import { toast } from 'sonner'; interface Props { - user: { id: string; email: string; full_name: string | null }; + email: string; + fullName: string; + onEmailChange: (value: string) => void; + onFullNameChange: (value: string) => void; + error?: string | null; } -export function DetailsCard({ user }: Props) { - const [email, setEmail] = useState(user.email); - const [fullName, setFullName] = useState(user.full_name ?? ''); - const [saving, setSaving] = useState(false); - const [error, setError] = useState(null); - - const handleSave = () => { - setSaving(true); - setError(null); - fetch(`/api/users/admin/${user.id}`, { - method: 'PATCH', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ email, full_name: fullName || null }), - }) - .then(async (res) => { - if (res.ok) { - toast.success('Details updated'); - } else { - const data = await res.json().catch(() => ({})); - setError(typeof data?.detail === 'string' ? data.detail : 'Failed to update details'); - } - }) - .catch(() => setError('An error occurred')) - .finally(() => setSaving(false)); - }; - +/** + * Editable account details. Fully controlled and without a save button of its + * own — the page owns one dirty state covering details and roles together. + */ +export function DetailsCard({ email, fullName, onEmailChange, onFullNameChange, error }: Props) { return ( - - {saving ? 'Saving…' : 'Save details'} - - } - > - Details - + Details
@@ -69,7 +41,7 @@ export function DetailsCard({ user }: Props) { id="edit-full-name" type="text" value={fullName} - onChange={(e) => setFullName(e.target.value)} + onChange={(e) => onFullNameChange(e.target.value)} placeholder="Jane Doe" />
diff --git a/modules/users/users/pages/Users/components/InviteFields.tsx b/modules/users/users/pages/Users/components/InviteFields.tsx new file mode 100644 index 00000000..508fda28 --- /dev/null +++ b/modules/users/users/pages/Users/components/InviteFields.tsx @@ -0,0 +1,46 @@ +import { Label } from '@simple-module-py/ui/components/ui/label'; +import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; +import { Info } from 'lucide-react'; + +interface Props { + emails: string; + onEmailsChange: (value: string) => void; + /** Addresses parsed out of the box so far. */ + count: number; + mailerDelivers: boolean; +} + +export function InviteFields({ emails, onEmailsChange, count, mailerDelivers }: Props) { + return ( +
+ +