From 5f843d8ce439a059632eebbcb0c42b8af017323d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 16:18:43 +0200 Subject: [PATCH 1/5] tests: tenant/soft-delete statement-shape matrix; sharing service and admin page; dashboard and audit-log page smoke tests Closes #597 Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/tests/_db_shape_matrix.py | 44 +++++++ .../db/tests/test_db_shape_matrix_dml.py | 35 ++++++ .../db/tests/test_db_shape_matrix_joins.py | 55 +++++++++ .../db/tests/test_db_shape_matrix_selects.py | 58 +++++++++ .../tests-js/audit_log_Browse.test.tsx | 81 +++++++++++++ .../tests-js/dashboard_Doctor.test.tsx | 41 +++++++ .../tests-js/dashboard_Home.test.tsx | 47 ++++++++ .../sharing/tests-js/sharing_Admin.test.tsx | 111 ++++++++++++++++++ modules/sharing/tests/test_sharing_service.py | 61 ++++++++++ 9 files changed, 533 insertions(+) create mode 100644 framework/db/tests/_db_shape_matrix.py create mode 100644 framework/db/tests/test_db_shape_matrix_dml.py create mode 100644 framework/db/tests/test_db_shape_matrix_joins.py create mode 100644 framework/db/tests/test_db_shape_matrix_selects.py create mode 100644 modules/audit_log/tests-js/audit_log_Browse.test.tsx create mode 100644 modules/dashboard/tests-js/dashboard_Doctor.test.tsx create mode 100644 modules/dashboard/tests-js/dashboard_Home.test.tsx create mode 100644 modules/sharing/tests-js/sharing_Admin.test.tsx diff --git a/framework/db/tests/_db_shape_matrix.py b/framework/db/tests/_db_shape_matrix.py new file mode 100644 index 000000000..1f2cd0081 --- /dev/null +++ b/framework/db/tests/_db_shape_matrix.py @@ -0,0 +1,44 @@ +"""Combined tenant/soft-delete probes shared by the statement families (#597).""" + +import pytest +from _models import _tenant_session +from simple_module_db.base import create_module_base +from simple_module_db.mixins import MultiTenantMixin, SoftDeleteMixin +from simple_module_db.tenancy import tenant_context +from sqlmodel import Field + +Base = create_module_base("shape_matrix") + + +class MatrixRow(Base, MultiTenantMixin, SoftDeleteMixin, table=True): # ty: ignore[unsupported-base] + __tablename__ = "shape_matrix_row" + id: int = Field(primary_key=True) + name: str = "original" + + +class MatrixAnchor(Base, table=True): # ty: ignore[unsupported-base] + __tablename__ = "shape_matrix_anchor" + id: int = Field(primary_key=True) + + +row, anchor = MatrixRow.__table__, MatrixAnchor.__table__ + + +async def seed(session): + session.add_all([MatrixAnchor(id=i) for i in range(1, 6)]) + for tenant, ids in (("A", (1, 2)), ("B", (3, 4))): + with tenant_context(tenant): + session.add_all([MatrixRow(id=ids[0]), MatrixRow(id=ids[1], is_deleted=True)]) + await session.flush() + await session.commit() + + +@pytest.fixture(params=[False, True], ids=["tenant-off", "tenant-on"]) +async def matrix_session(request): + async for session in _tenant_session(Base.metadata, strict=request.param, seed=seed): + yield session + + +def visible_ids(tenant, include_deleted): + ids = {"A": [1, 2], "B": [3, 4], None: [1, 2, 3, 4]}[tenant] + return ids if include_deleted else [i for i in ids if i % 2] diff --git a/framework/db/tests/test_db_shape_matrix_dml.py b/framework/db/tests/test_db_shape_matrix_dml.py new file mode 100644 index 000000000..1ff024ca9 --- /dev/null +++ b/framework/db/tests/test_db_shape_matrix_dml.py @@ -0,0 +1,35 @@ +"""Bulk DML scopes tenants but intentionally includes trashed targets (#597).""" + +import pytest +from _db_shape_matrix import matrix_session, row, visible_ids +from simple_module_db.query_filter import strict_configured +from simple_module_db.tenancy import TenantIsolationError, no_tenant, tenant_context +from sqlalchemy import delete, select, update + +__all__ = ["matrix_session"] + + +@pytest.mark.parametrize("operation", ["update", "delete"]) +@pytest.mark.parametrize("tenant", ["A", "B", None]) +@pytest.mark.parametrize("include_deleted", [False, True]) +async def test_dml_matrix(matrix_session, operation, tenant, include_deleted): + stmt = update(row).values(name="changed") if operation == "update" else delete(row) + stmt = stmt.execution_options(include_deleted=include_deleted) + with no_tenant() if tenant is None else tenant_context(tenant): + if tenant is None and strict_configured(matrix_session.sync_session): + with pytest.raises(TenantIsolationError): + await matrix_session.execute(stmt) + affected = [] + else: + result = await matrix_session.execute(stmt) + affected = visible_ids(tenant, True) + assert result.rowcount == len(affected) + # Bypass both filters to prove *which* rows changed, including foreign/deleted rows. + inspect = select(row.c.id, row.c.name).execution_options(all_tenants=True, include_deleted=True) + actual = sorted((await matrix_session.execute(inspect)).all()) + expected = [ + (i, "changed" if i in affected else "original") + for i in range(1, 5) + if operation == "update" or i not in affected + ] + assert actual == expected diff --git a/framework/db/tests/test_db_shape_matrix_joins.py b/framework/db/tests/test_db_shape_matrix_joins.py new file mode 100644 index 000000000..164755b6e --- /dev/null +++ b/framework/db/tests/test_db_shape_matrix_joins.py @@ -0,0 +1,55 @@ +"""Join predicates must filter nullable rows without losing their anchors (#597).""" + +import pytest +from _db_shape_matrix import anchor, matrix_session, row, visible_ids +from simple_module_db._core_joins import scope_joins +from simple_module_db.query_filter import strict_configured +from simple_module_db.tenancy import TenantIsolationError, no_tenant, tenant_context +from sqlalchemy import select + +__all__ = ["matrix_session"] + + +@pytest.mark.parametrize("outer", [False, True], ids=["join", "outerjoin"]) +@pytest.mark.parametrize("setup", [False, True], ids=["explicit", "setup"]) +@pytest.mark.parametrize("tenant", ["A", "B", None]) +@pytest.mark.parametrize("include_deleted", [False, True]) +async def test_join_matrix(matrix_session, outer, setup, tenant, include_deleted): + on = row.c.id == anchor.c.id + stmt = select(anchor.c.id, row.c.id) + if setup: + stmt = stmt.select_from(anchor).join(row, on, isouter=outer) + else: + stmt = stmt.select_from(anchor.join(row, on, isouter=outer)) + stmt = stmt.execution_options(include_deleted=include_deleted) + with no_tenant() if tenant is None else tenant_context(tenant): + if tenant is None and strict_configured(matrix_session.sync_session): + with pytest.raises(TenantIsolationError): + await matrix_session.execute(stmt) + else: + visible = visible_ids(tenant, include_deleted) + expected = ( + [(i, i if i in visible else None) for i in range(1, 6)] + if outer + else [(i, i) for i in visible] + ) + assert sorted((await matrix_session.execute(stmt)).all()) == expected + + +@pytest.mark.parametrize("outer", [False, True]) +def test_scope_joins_places_nullable_predicate_in_on(outer): + stmt = select(anchor.c.id, row.c.id).select_from( + anchor.join(row, anchor.c.id == row.c.id, isouter=outer) + ) + scoped = scope_joins( + stmt, lambda table: [table.c.is_deleted.is_(False)] if table is row else [] + ) + assert scoped.covered == [row] + assert bool(scoped.where) is (not outer) + rewritten = stmt._generate() + rewritten._from_obj = scoped.from_obj + rewritten = rewritten.where(*scoped.where) + sql = str(rewritten) + before_where, _, where = sql.partition("WHERE") + assert ("is_deleted" in before_where) is outer + assert ("is_deleted" in where) is (not outer) diff --git a/framework/db/tests/test_db_shape_matrix_selects.py b/framework/db/tests/test_db_shape_matrix_selects.py new file mode 100644 index 000000000..66d13be67 --- /dev/null +++ b/framework/db/tests/test_db_shape_matrix_selects.py @@ -0,0 +1,58 @@ +"""Select/walk matrix, using the same fixture on SQLite and Postgres (#597).""" + +import pytest +from _db_shape_matrix import MatrixRow, anchor, matrix_session, row, visible_ids +from simple_module_db._core_walk import references_plain +from simple_module_db.query_filter import strict_configured +from simple_module_db.tenancy import TenantIsolationError, no_tenant, tenant_context +from sqlalchemy import delete, exists, select, union, update + +__all__ = ["matrix_session"] + + +def statement(shape): + if shape == "orm": + return select(MatrixRow.id) + if shape == "core": + return select(row.c.id) + if shape == "select_from": + return select(anchor.c.id).select_from(anchor, row).where(anchor.c.id == row.c.id) + if shape == "subquery": + return select(select(row.c.id).subquery().c.id) + if shape == "cte": + return select(select(row.c.id).cte("scoped").c.id) + if shape == "union": + return union(select(row.c.id).where(row.c.id <= 2), select(row.c.id).where(row.c.id > 2)) + if shape == "exists": + return select(anchor.c.id).where(exists().where(row.c.id == anchor.c.id)) + raise AssertionError(shape) + + +SHAPES = ["orm", "core", "select_from", "subquery", "cte", "union", "exists"] + + +@pytest.mark.parametrize("shape", SHAPES) +@pytest.mark.parametrize("tenant", ["A", "B", None], ids=["own", "other", "unbound"]) +@pytest.mark.parametrize("include_deleted", [False, True]) +async def test_select_matrix(matrix_session, shape, tenant, include_deleted): + stmt = statement(shape).execution_options(include_deleted=include_deleted) + with no_tenant() if tenant is None else tenant_context(tenant): + if tenant is None and strict_configured(matrix_session.sync_session): + with pytest.raises(TenantIsolationError): + await matrix_session.execute(stmt) + else: + ids = (await matrix_session.execute(stmt)).scalars().all() + assert sorted(ids) == visible_ids(tenant, include_deleted) + + +@pytest.mark.parametrize("shape", [*SHAPES, "update", "delete"]) +def test_walk_detects_only_plain_scoped_references(shape): + stmt = ( + update(row).values(name="changed") + if shape == "update" + else delete(row) + if shape == "delete" + else statement(shape) + ) + assert references_plain(stmt, {row.name}) is (shape != "orm") + assert references_plain(stmt, {"unrelated_table"}) is False diff --git a/modules/audit_log/tests-js/audit_log_Browse.test.tsx b/modules/audit_log/tests-js/audit_log_Browse.test.tsx new file mode 100644 index 000000000..04a87017f --- /dev/null +++ b/modules/audit_log/tests-js/audit_log_Browse.test.tsx @@ -0,0 +1,81 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { render, screen } from '@testing-library/react'; +import type { ComponentProps } from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ href, children }: { href: string; children: React.ReactNode }) => ( + {children} + ), + router: { visit: vi.fn() }, + usePage: () => ({ props: {} }), +})); + +import Browse from '../audit_log/pages/Browse'; + +const props: ComponentProps = { + items: [], + total: 0, + page: 1, + page_size: 50, + entity_types: [], + tenant_ids: [], + platform_tenant_value: '__platform__', + export_url: '/api/audit_log/export', + filters: { + entity_type: null, + action: null, + tenant_id: null, + user_id: null, + correlation_id: null, + from_date: null, + to_date: null, + }, +}; + +describe('audit_log Browse', () => { + it('mounts the filter form, empty state, and export link', () => { + render(); + expect(screen.getByRole('heading', { name: keys.audit_log.browse.title })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: keys.audit_log.filters.apply })).toBeInTheDocument(); + expect(screen.getByRole('link', { name: keys.audit_log.browse.export_csv })).toHaveAttribute( + 'download', + ); + expect(screen.getByRole('button', { name: keys.ui.pagination.next })).toBeDisabled(); + }); + + it('mounts a populated entry with its resolved actor and resource', () => { + render( + , + ); + expect(screen.getByText('Alice')).toBeInTheDocument(); + expect(screen.getByText('Meeting notes')).toBeInTheDocument(); + expect(screen.getByRole('table')).toBeInTheDocument(); + }); +}); diff --git a/modules/dashboard/tests-js/dashboard_Doctor.test.tsx b/modules/dashboard/tests-js/dashboard_Doctor.test.tsx new file mode 100644 index 000000000..87c32c13b --- /dev/null +++ b/modules/dashboard/tests-js/dashboard_Doctor.test.tsx @@ -0,0 +1,41 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { fireEvent, render, screen } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); +const post = vi.hoisted(() => vi.fn()); +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + router: { post }, + usePage: () => ({ props: {} }), +})); + +import type { DoctorProps } from '../dashboard/pages/components/doctor/types'; +import Doctor from '../dashboard/pages/Doctor'; + +const props: DoctorProps = { + checks: [{ id: 'pages', status: 'pass', command: null, findings: [] }], + checks_available: true, + migrations: [{ id: 'a3f1beef', module: 'users', message: 'add invite table', applied: true }], + migration_commands: { generate: 'make migration', apply: 'make migrate' }, + dev_server: { running: true, rows: [{ name: 'vite', value: ':5050' }] }, + pages_routed: 26, + stats: { + checks_passing: 1, + checks_total: 1, + modules_loaded: 12, + pending_migrations: 0, + python_version: '3.12.4', + }, +}; + +describe('dashboard Doctor', () => { + it.each([true, false])('mounts with diagnostics available=%s', (checks_available) => { + render(); + expect(screen.getByRole('heading', { name: keys.dashboard.doctor.title })).toBeInTheDocument(); + expect(screen.getByTitle('a3f1beef')).toHaveTextContent('a3f1'); + expect(screen.getAllByText(':5050').length).toBeGreaterThan(0); + fireEvent.click(screen.getByRole('button', { name: keys.dashboard.doctor.rerun })); + expect(post).toHaveBeenCalledWith('/admin/doctor/rerun'); + }); +}); diff --git a/modules/dashboard/tests-js/dashboard_Home.test.tsx b/modules/dashboard/tests-js/dashboard_Home.test.tsx new file mode 100644 index 000000000..f650c5fa1 --- /dev/null +++ b/modules/dashboard/tests-js/dashboard_Home.test.tsx @@ -0,0 +1,47 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { render, screen } from '@testing-library/react'; +import type { ComponentProps } from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ href, children }: { href: string; children: React.ReactNode }) => ( + {children} + ), + usePage: () => ({ props: { menus: {} } }), +})); + +import Home from '../dashboard/pages/Home'; + +const props: ComponentProps = { + welcome: 'Welcome back', + can_view_stats: true, + total_users: 12, + active_users_7d: 7, + users_created_this_month: 2, + module_count: 1, + system_info: { + modules: [ + { name: 'Mail', package: 'mail', status: 'loaded', url: '', admin_url: '', health: '' }, + ], + python_version: '3.12.4', + health_checks: [], + }, +}; + +describe('dashboard Home', () => { + it('mounts the stats and system module deck', () => { + render(); + expect(screen.getByRole('heading', { name: keys.dashboard.home.title })).toBeInTheDocument(); + expect(screen.getByText(keys.dashboard.home.stats.total_users)).toBeInTheDocument(); + expect(screen.getByText('mail')).toBeInTheDocument(); + }); + + it('mounts only the welcome content without stats permission', () => { + render(); + expect(screen.getByText('Welcome back')).toBeInTheDocument(); + expect(screen.queryByText(keys.dashboard.home.stats.total_users)).toBeNull(); + expect(screen.queryByText('mail')).toBeNull(); + }); +}); diff --git a/modules/sharing/tests-js/sharing_Admin.test.tsx b/modules/sharing/tests-js/sharing_Admin.test.tsx new file mode 100644 index 000000000..eadb6eb83 --- /dev/null +++ b/modules/sharing/tests-js/sharing_Admin.test.tsx @@ -0,0 +1,111 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import type { ComponentProps } from 'react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); +const { post, get } = vi.hoisted(() => ({ post: vi.fn(), get: vi.fn() })); +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + router: { post, get }, + usePage: () => ({ props: {} }), +})); + +import Admin from '../sharing/pages/Admin'; + +const props: ComponentProps = { + shares: [ + { + id: 'share-1', + resource_type: 'notes.note', + resource_id: 'note-1', + subject_type: 'user', + subject_id: 'friend', + subject_label: 'Friend', + role: 'view', + granted_by: 'owner', + granted_by_label: 'Owner', + expires_at: null, + expired: false, + created_at: null, + }, + ], + total: 21, + page: 2, + per_page: 10, + can_revoke: true, + tenant_required: false, +}; + +afterEach(() => vi.clearAllMocks()); + +describe('sharing Admin', () => { + it('renders labels and hides revoke from read-only viewers', () => { + render(); + expect(screen.getByText('Friend')).toBeInTheDocument(); + expect(screen.getByText('Owner')).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: keys.sharing.list.revoke })).toBeNull(); + }); + + it('shows the tenant warning instead of shares and pagination when unbound', () => { + render(); + expect(screen.getByText(keys.sharing.admin.tenant_required)).toBeInTheDocument(); + expect(screen.queryByText('Friend')).toBeNull(); + expect(screen.queryByRole('button', { name: keys.ui.pagination.next })).toBeNull(); + }); + + it('shows an empty state without pagination for an empty list', () => { + render(); + expect(screen.getByText(keys.sharing.list.empty_title)).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: keys.ui.pagination.next })).toBeNull(); + }); + + it.each([ + [keys.ui.pagination.previous, 1], + [keys.ui.pagination.next, 3], + ])('navigates using %s', (name, page) => { + render(); + fireEvent.click(screen.getByRole('button', { name })); + expect(get).toHaveBeenCalledWith('/admin/sharing/', { page }, { preserveScroll: true }); + }); + + it.each([1, 3])('disables navigation beyond page %s', (page) => { + render(); + const name = page === 1 ? keys.ui.pagination.previous : keys.ui.pagination.next; + expect(screen.getByRole('button', { name })).toBeDisabled(); + }); + + it('cancels without writing and revokes only after confirmation', async () => { + render(); + fireEvent.click(screen.getByRole('button', { name: keys.sharing.list.revoke })); + expect(post).not.toHaveBeenCalled(); + fireEvent.click(await screen.findByRole('button', { name: keys.sharing.revoke.cancel })); + await waitFor(() => expect(screen.queryByText(keys.sharing.revoke.description)).toBeNull()); + expect(post).not.toHaveBeenCalled(); + fireEvent.click(screen.getByRole('button', { name: keys.sharing.list.revoke })); + fireEvent.click(await screen.findByRole('button', { name: keys.sharing.revoke.confirm })); + expect(post).toHaveBeenCalledWith( + '/admin/sharing/share-1/revoke', + {}, + { + preserveScroll: true, + onFinish: expect.any(Function), + }, + ); + expect(screen.getByRole('button', { name: keys.sharing.revoke.confirm })).toBeDisabled(); + act(() => post.mock.calls[0][2].onFinish()); + await waitFor(() => expect(screen.queryByText(keys.sharing.revoke.description)).toBeNull()); + }); + + it('clears pending state on visit completion so an unchanged share can be retried', async () => { + // This page delegates errors to Inertia; it has no local error-toast callback. + render(); + fireEvent.click(screen.getByRole('button', { name: keys.sharing.list.revoke })); + fireEvent.click(await screen.findByRole('button', { name: keys.sharing.revoke.confirm })); + act(() => post.mock.calls[0][2].onFinish()); + await waitFor(() => expect(screen.queryByText(keys.sharing.revoke.description)).toBeNull()); + expect(screen.getByText('Friend')).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: keys.sharing.list.revoke })); + expect(await screen.findByRole('button', { name: keys.sharing.revoke.confirm })).toBeEnabled(); + }); +}); diff --git a/modules/sharing/tests/test_sharing_service.py b/modules/sharing/tests/test_sharing_service.py index 368fc8ff3..21fef5f64 100644 --- a/modules/sharing/tests/test_sharing_service.py +++ b/modules/sharing/tests/test_sharing_service.py @@ -14,6 +14,7 @@ create_note_table, register_note_type, ) +from sharing.constants import PERM_MANAGE from sharing.contracts.schemas import ShareGrant from sharing.models import ResourceShare from sharing.service import SharingError, SharingService @@ -143,3 +144,63 @@ async def test_grantee_may_leave_and_owner_lists(env): await _refused(service.for_resource(_who(FRIEND), NOTE_TYPE, note_id), 403) await service.revoke(_who(FRIEND), share.id) assert await _visible(service.db, FRIEND) == [] + + +@pytest.mark.parametrize("role", [ACTION_VIEW, ACTION_EDIT]) +async def test_grantee_cannot_manage_shares_even_with_edit_role(env, role): + service, note_id = env + with tenant_context("t1"): + await service.grant(_who(OWNER), _grant(note_id, role=role)) + reshare = _grant(note_id).model_copy(update={"subject_id": STRANGER}) + await _refused(service.grant(_who(FRIEND), reshare), 403) + await _refused(service.for_resource(_who(FRIEND), NOTE_TYPE, note_id), 403) + assert len(await service.for_resource(_who(OWNER), NOTE_TYPE, note_id)) == 1 + + +@pytest.mark.parametrize("actor", [OWNER, FRIEND, "admin"]) +async def test_real_share_id_cannot_be_revoked_across_tenants(env, actor): + service, note_id = env + who = ( + ResourcePrincipal(user_id=actor, permissions=frozenset({PERM_MANAGE})) + if actor == "admin" + else _who(actor) + ) + with tenant_context("t1"): + share = await service.grant(_who(OWNER), _grant(note_id)) + with tenant_context("t2"): + await _refused(service.revoke(who, share.id), 404) + with tenant_context("t1"): + assert [s.id for s in await service.for_resource(_who(OWNER), NOTE_TYPE, note_id)] == [ + share.id + ] + assert await _visible(service.db, FRIEND) == ["n"] + + +async def test_admin_may_revoke_without_resource_ownership(env): + service, note_id = env + admin = ResourcePrincipal(user_id=STRANGER, permissions=frozenset({PERM_MANAGE})) + with tenant_context("t1"): + share = await service.grant(_who(OWNER), _grant(note_id)) + await service.revoke(admin, share.id) + assert await service.for_resource(_who(OWNER), NOTE_TYPE, note_id) == [] + assert await _visible(service.db, FRIEND) == [] + await _refused(service.revoke(admin, share.id), 404) + + +async def test_expired_share_can_be_renewed_without_duplicate_grants(env): + service, note_id = env + with tenant_context("t1"): + share = await service.grant(_who(OWNER), _grant(note_id)) + share.expires_at = datetime.now(UTC) - timedelta(days=1) + await service.db.flush() + assert await _visible(service.db, FRIEND) == [] + assert [s.id for s in await service.for_resource(_who(OWNER), NOTE_TYPE, note_id)] == [ + share.id + ] + future = datetime.now(UTC) + timedelta(days=1) + renewed = await service.grant( + _who(OWNER), _grant(note_id, expires_at=future, role=ACTION_EDIT) + ) + assert renewed.id == share.id + assert await _visible(service.db, FRIEND, ACTION_EDIT) == ["n"] + assert len(await service.for_resource(_who(OWNER), NOTE_TYPE, note_id)) == 1 From 958eaf80a115da0e5d88911c1abeb90e8ed53b30 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 16:18:43 +0200 Subject: [PATCH 2/5] frontend: drop unused deps, consistent failure toasts, shared leave guard and confirm dialog, client error reporting hook Closes #601 Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/frontend/inertia.md | 26 +-- host/client_app/package.json | 5 +- .../background_tasks/pages/Outbox.tsx | 30 ++-- .../background_tasks_OutboxPage.test.tsx | 18 ++- modules/dashboard/dashboard/locales/de.json | 3 +- modules/dashboard/dashboard/locales/en.json | 3 +- modules/dashboard/dashboard/locales/es.json | 3 +- modules/dashboard/dashboard/pages/Doctor.tsx | 16 +- .../tests-js/dashboard_Doctor_errors.test.tsx | 44 ++++++ .../permissions/pages/RoleEdit.tsx | 2 +- .../permissions/pages/UserEdit.tsx | 2 +- modules/sharing/sharing/locales/de.json | 3 +- modules/sharing/sharing/locales/en.json | 3 +- modules/sharing/sharing/pages/Admin.tsx | 8 + .../tests-js/sharing_Admin_errors.test.tsx | 61 +++++++ .../pages/Users/components/useEditUserForm.ts | 18 +-- package-lock.json | 141 +---------------- packages/i18n/src/generated-resources.ts | 2 + packages/i18n/src/keys.generated.ts | 2 + packages/ui/README.md | 2 +- packages/ui/package.json | 1 - packages/ui/src/components/ErrorBoundary.tsx | 5 +- packages/ui/src/components/ui/form.tsx | 149 ------------------ .../src/components/ui_ErrorBoundary.test.tsx | 30 ++++ .../ui/src/hooks/ui_useLeaveGuard.test.tsx | 2 +- .../ui/src/hooks}/useLeaveGuard.ts | 0 packages/ui/src/index.ts | 2 + packages/ui/src/lib/client-errors.ts | 6 + 28 files changed, 240 insertions(+), 347 deletions(-) create mode 100644 modules/dashboard/tests-js/dashboard_Doctor_errors.test.tsx create mode 100644 modules/sharing/tests-js/sharing_Admin_errors.test.tsx delete mode 100644 packages/ui/src/components/ui/form.tsx create mode 100644 packages/ui/src/components/ui_ErrorBoundary.test.tsx rename modules/permissions/tests-js/useLeaveGuard.test.tsx => packages/ui/src/hooks/ui_useLeaveGuard.test.tsx (97%) rename {modules/permissions/permissions/pages/components => packages/ui/src/hooks}/useLeaveGuard.ts (100%) create mode 100644 packages/ui/src/lib/client-errors.ts diff --git a/docs/frontend/inertia.md b/docs/frontend/inertia.md index 6baf20240..cbee069e4 100644 --- a/docs/frontend/inertia.md +++ b/docs/frontend/inertia.md @@ -39,25 +39,30 @@ Breakdown: ## Responding with JSON vs. Inertia -- **`endpoints/api.py`** — returns JSON. Called by background/non-Inertia code, or by Inertia's `router.get/post` on the client (which sends `X-Inertia: true` and receives a full Inertia response, **not** raw JSON). -- **`endpoints/views.py`** — returns Inertia pages. Routed at `view_prefix`, rendered as full HTML on the first request and as JSON page updates on client-side navigation. - -The split is convention: keep GET-for-page-data calls in `views.py`, keep POST/PATCH/DELETE data mutations in `api.py` even if triggered from Inertia forms. This way JSON APIs are usable from scripts and Inertia pages route cleanly. +- **`endpoints/api.py`** returns JSON. Use the typed `createApi` client wrapped in + `apiResult` for JSON actions, and toast failed results (including network failures). +- **`endpoints/views.py`** returns Inertia pages or redirects. View-route forms use + Inertia `useForm` for data, validation errors and processing state. Small actions + without form fields may use `router.post/delete` with an `onError` toast. + Handle `onHttpException` and `onNetworkError` too when an action should toast + HTTP or connection failures; returning `false` suppresses Inertia's default handler. ## Inertia form submission -From a page, use Inertia's `router.post`: +Submit to the module's view route: ```tsx -import { router } from "@inertiajs/react"; +import { useForm } from "@inertiajs/react"; function CreateOrder() { - const [data, setData] = useState({ customer_email: "", total: "" }); + const form = useForm({ customer_email: "", total: "" }); return (
{ e.preventDefault(); - router.post("/api/orders", data); + form.post("/orders", { + onError: () => toast.error(t(keys.orders.create.failed)), + }); }} > ... @@ -97,8 +102,9 @@ There is no `flash` shared prop. For "created successfully" toasts, fire `sonner ```tsx import { toast } from "sonner"; -router.post("/api/orders", data, { - onSuccess: () => toast.success("Order created"), +router.post("/orders", data, { + onSuccess: () => toast.success(t(keys.orders.create.saved)), + onError: () => toast.error(t(keys.orders.create.failed)), }); ``` diff --git a/host/client_app/package.json b/host/client_app/package.json index 9042739d5..8be09cf96 100644 --- a/host/client_app/package.json +++ b/host/client_app/package.json @@ -8,15 +8,12 @@ "preview": "vite preview" }, "dependencies": { - "@hookform/resolvers": "^5.9.1", "@inertiajs/react": "^3.7.0", "@simple-module-py/i18n": "*", "@simple-module-py/ui": "*", - "date-fns": "^4.4.0", "lucide-react": "^1.43.0", "react": "^19.2.8", - "react-dom": "^19.2.8", - "zod": "^4.5.4" + "react-dom": "^19.2.8" }, "devDependencies": { "@simple-module-py/tsconfig": "*", diff --git a/modules/background_tasks/background_tasks/pages/Outbox.tsx b/modules/background_tasks/background_tasks/pages/Outbox.tsx index 0c747d282..cb4956499 100644 --- a/modules/background_tasks/background_tasks/pages/Outbox.tsx +++ b/modules/background_tasks/background_tasks/pages/Outbox.tsx @@ -1,11 +1,12 @@ import { Head, Link, router } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; +import { ConfirmActionDialog } from '@simple-module-py/ui/components/ConfirmActionDialog'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { usePermissions } from '@simple-module-py/ui/hooks/use-permissions'; import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { cn } from '@simple-module-py/ui/lib/utils'; -import { ArrowLeft } from 'lucide-react'; +import { ArrowLeft, Trash2 } from 'lucide-react'; import { useState } from 'react'; import { OutboxTable } from './components/OutboxTable'; import { VIEW_BASE } from './constants'; @@ -31,6 +32,7 @@ function Outbox({ messages, counts, pagination, filters }: Props) { const { t } = useT(); const { can } = usePermissions(); const canManage = can('background_tasks.manage'); + const [pendingDiscard, setPendingDiscard] = useState(null); const [busyId, setBusyId] = useState(null); function visit(status: string, page: number) { @@ -51,13 +53,6 @@ function Outbox({ messages, counts, pagination, filters }: Props) { } } - function discard(message: OutboxMessage) { - const prompt = t(keys.background_tasks.outbox.discard_confirm, { - handler: message.handler_name, - }); - if (window.confirm(prompt)) void act(message, discardOutboxMessage); - } - const segments = [ { value: '', label: t(keys.background_tasks.outbox.filter_all), count: null }, ...OPEN_OUTBOX_STATUSES.map((s) => ({ @@ -105,13 +100,30 @@ function Outbox({ messages, counts, pagination, filters }: Props) { canManage={canManage} busyId={busyId} onRetry={(m) => void act(m, retryOutboxMessage)} - onDiscard={discard} + onDiscard={setPendingDiscard} page={pagination.page} perPage={pagination.per_page} total={pagination.total} onPageChange={(page) => visit(filters.status, page)} /> + !open && setPendingDiscard(null)} + icon={Trash2} + title={t(keys.background_tasks.outbox.discard)} + description={t(keys.background_tasks.outbox.discard_confirm, { + handler: pendingDiscard?.handler_name ?? '', + })} + confirmLabel={t(keys.background_tasks.outbox.discard)} + cancelLabel={t(keys.background_tasks.retry_dialog.cancel)} + busy={busyId !== null} + onConfirm={() => { + if (pendingDiscard) { + void act(pendingDiscard, discardOutboxMessage).finally(() => setPendingDiscard(null)); + } + }} + /> ); } diff --git a/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx b/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx index a289d7fd1..a3f80ca92 100644 --- a/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx +++ b/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx @@ -1,6 +1,6 @@ import '@testing-library/jest-dom/vitest'; import { configureI18n, keys } from '@simple-module-py/i18n'; -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor, within } from '@testing-library/react'; import type { ComponentProps } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -134,21 +134,27 @@ describe('background_tasks Outbox page', () => { it('asks before discarding and calls nothing when the operator declines', () => { const fetchMock = stubFetch(204); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); render()} />); fireEvent.click(screen.getByRole('button', { name: ob.discard })); - expect(confirmSpy).toHaveBeenCalledTimes(1); + const dialog = screen.getByRole('alertdialog'); + fireEvent.click( + within(dialog).getByRole('button', { name: keys.background_tasks.retry_dialog.cancel }), + ); + expect(screen.queryByRole('alertdialog')).toBeNull(); expect(fetchMock).not.toHaveBeenCalled(); expect(reload).not.toHaveBeenCalled(); }); it('discards with a DELETE once confirmed', async () => { const fetchMock = stubFetch(204); - vi.spyOn(window, 'confirm').mockReturnValue(true); render()} />); fireEvent.click(screen.getByRole('button', { name: ob.discard })); + expect(fetchMock).not.toHaveBeenCalled(); + fireEvent.click( + within(screen.getByRole('alertdialog')).getByRole('button', { name: ob.discard }), + ); await waitFor(() => expect(toast.success).toHaveBeenCalledWith(ob.discarded)); expect(requestsOf(fetchMock)).toContainEqual({ url: '/api/background_tasks/admin/outbox/m1', @@ -159,9 +165,11 @@ describe('background_tasks Outbox page', () => { it('toasts a discard failure', async () => { stubFetch(500); - vi.spyOn(window, 'confirm').mockReturnValue(true); render()} />); fireEvent.click(screen.getByRole('button', { name: ob.discard })); + fireEvent.click( + within(screen.getByRole('alertdialog')).getByRole('button', { name: ob.discard }), + ); await waitFor(() => expect(toast.error).toHaveBeenCalledWith(ob.discard_failed)); }); diff --git a/modules/dashboard/dashboard/locales/de.json b/modules/dashboard/dashboard/locales/de.json index c3fbedee4..9294a20ff 100644 --- a/modules/dashboard/dashboard/locales/de.json +++ b/modules/dashboard/dashboard/locales/de.json @@ -79,7 +79,8 @@ "report_checks": "Prüfungen", "report_checks_unavailable": "nicht verfügbar (Diagnose läuft nur in der Entwicklungsumgebung)", "report_migrations": "Migrationen", - "report_dev_server": "Entwicklungsserver" + "report_dev_server": "Entwicklungsserver", + "rerun_failed": "Die Diagnose konnte nicht erneut ausgeführt werden" }, "nav": { "dashboard": "Dashboard", diff --git a/modules/dashboard/dashboard/locales/en.json b/modules/dashboard/dashboard/locales/en.json index 39194916e..a8a784406 100644 --- a/modules/dashboard/dashboard/locales/en.json +++ b/modules/dashboard/dashboard/locales/en.json @@ -79,7 +79,8 @@ "report_checks": "Checks", "report_checks_unavailable": "not available (diagnostics run in development only)", "report_migrations": "Migrations", - "report_dev_server": "Dev server" + "report_dev_server": "Dev server", + "rerun_failed": "Failed to rerun diagnostics" }, "nav": { "dashboard": "Dashboard", diff --git a/modules/dashboard/dashboard/locales/es.json b/modules/dashboard/dashboard/locales/es.json index a2349884e..80b9c32ac 100644 --- a/modules/dashboard/dashboard/locales/es.json +++ b/modules/dashboard/dashboard/locales/es.json @@ -79,7 +79,8 @@ "report_checks": "Comprobaciones", "report_checks_unavailable": "no disponible (las comprobaciones solo se ejecutan en desarrollo)", "report_migrations": "Migraciones", - "report_dev_server": "Servidor de desarrollo" + "report_dev_server": "Servidor de desarrollo", + "rerun_failed": "No se pudo volver a ejecutar el diagnóstico" }, "nav": { "dashboard": "Panel", diff --git a/modules/dashboard/dashboard/pages/Doctor.tsx b/modules/dashboard/dashboard/pages/Doctor.tsx index b3ec88ac5..904b2bac7 100644 --- a/modules/dashboard/dashboard/pages/Doctor.tsx +++ b/modules/dashboard/dashboard/pages/Doctor.tsx @@ -20,6 +20,10 @@ const RERUN_URL = '/admin/doctor/rerun'; function Doctor(props: DoctorProps) { const { t } = useT(); const checkLabels = useCheckLabels(); + const rerunFailed = () => { + toast.error(t(keys.dashboard.doctor.rerun_failed)); + return false; + }; async function copy(text: string, successMessage: string) { if (await copyToClipboard(text)) { @@ -69,7 +73,17 @@ function Doctor(props: DoctorProps) {