From cc7690bf05df831f96734699711e0695e591c70a Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 15:46:37 +0200 Subject: [PATCH 1/4] fix: background_tasks: settings form accepts unbounded numeric values and gives no save feedback (#270) --- biome.json | 1 + docs/modules/background_tasks.md | 4 +- .../background_tasks/settings.py | 14 ++++- .../background_tasks/tests/test_settings.py | 27 +++++++++ modules/settings/package.json | 4 +- .../settings/pages/components/ModuleForm.tsx | 2 + modules/settings/tests-js/ModuleForm.test.tsx | 60 +++++++++++++++++++ modules/settings/tests/test_module_api.py | 22 +++++++ modules/settings/tsconfig.json | 2 +- modules/users/tests-js/invite-emails.test.tsx | 41 +++++++++++++ modules/users/tsconfig.json | 2 +- modules/users/users/pages/Users/AddPeople.tsx | 18 +++--- .../pages/Users/components/InviteFields.tsx | 19 +++++- .../users/users/pages/Users/invite-emails.ts | 17 ++++++ package-lock.json | 3 + vitest.config.mts | 2 + 16 files changed, 221 insertions(+), 17 deletions(-) create mode 100644 modules/background_tasks/tests/test_settings.py create mode 100644 modules/settings/tests-js/ModuleForm.test.tsx create mode 100644 modules/users/tests-js/invite-emails.test.tsx create mode 100644 modules/users/users/pages/Users/invite-emails.ts diff --git a/biome.json b/biome.json index d503df20..2b175287 100644 --- a/biome.json +++ b/biome.json @@ -6,6 +6,7 @@ "packages/**", "modules/*/*/pages/**", "modules/*/*/components/**", + "modules/*/tests-js/**", "!host/client_app/modules.generated.ts", "!host/client_app/modules.manifest.json", "!host/client_app/modules.generated.css", diff --git a/docs/modules/background_tasks.md b/docs/modules/background_tasks.md index 0772e693..29c10a9a 100644 --- a/docs/modules/background_tasks.md +++ b/docs/modules/background_tasks.md @@ -148,8 +148,8 @@ DB-backed; defaults are in `BackgroundTasksSettings`. Several are marked `requir | `stuck_after_seconds` | `300` | heartbeat-staleness threshold | | `stuck_sweep_interval_seconds` | `60` | beat cadence for the stuck sweep | | `purge_interval_seconds` | `86_400` | beat cadence for old-row purge | -| `retention_days` | `14` | how long to keep terminal rows | -| `max_retries` | `3` | informational; tasks define their own policies | +| `retention_days` | `14` | how long to keep terminal rows; range 1-3650 | +| `max_retries` | `3` | informational; tasks define their own policies; range 0-100 | Bootstrap env-var equivalents (`SM_BG_TASKS_*`) only seed pydantic defaults at first boot — once a value lives in the DB, it's authoritative. diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index 63d84a6d..d052ea83 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -63,8 +63,18 @@ class BackgroundTasksSettings(BaseSettings): stuck_sweep_interval_seconds: int = DEFAULT_STUCK_SWEEP_INTERVAL_SECONDS purge_interval_seconds: int = DEFAULT_PURGE_INTERVAL_SECONDS - retention_days: int = DEFAULT_RETENTION_DAYS - max_retries: int = DEFAULT_MAX_RETRIES + retention_days: int = Field( + default=DEFAULT_RETENTION_DAYS, + ge=1, + le=3650, + description="Days to keep terminal task execution records (1-3650).", + ) + max_retries: int = Field( + default=DEFAULT_MAX_RETRIES, + ge=0, + le=100, + description="Configured retry ceiling; individual tasks define their own policies (0-100).", + ) @model_validator(mode="after") def _forbid_localhost_broker_in_production(self) -> BackgroundTasksSettings: diff --git a/modules/background_tasks/tests/test_settings.py b/modules/background_tasks/tests/test_settings.py new file mode 100644 index 00000000..d488fa41 --- /dev/null +++ b/modules/background_tasks/tests/test_settings.py @@ -0,0 +1,27 @@ +from __future__ import annotations + +import pytest +from background_tasks.settings import BackgroundTasksSettings +from pydantic import ValidationError + + +@pytest.mark.parametrize("retention_days", [1, 3650]) +def test_retention_days_accepts_supported_boundaries(retention_days: int) -> None: + assert BackgroundTasksSettings(retention_days=retention_days).retention_days == retention_days + + +@pytest.mark.parametrize("retention_days", [0, -5, 3651, 999_999_999]) +def test_retention_days_rejects_values_outside_supported_range(retention_days: int) -> None: + with pytest.raises(ValidationError): + BackgroundTasksSettings(retention_days=retention_days) + + +@pytest.mark.parametrize("max_retries", [0, 100]) +def test_max_retries_accepts_supported_boundaries(max_retries: int) -> None: + assert BackgroundTasksSettings(max_retries=max_retries).max_retries == max_retries + + +@pytest.mark.parametrize("max_retries", [-1, 101, 999_999_999]) +def test_max_retries_rejects_values_outside_supported_range(max_retries: int) -> None: + with pytest.raises(ValidationError): + BackgroundTasksSettings(max_retries=max_retries) diff --git a/modules/settings/package.json b/modules/settings/package.json index 52830f7a..3b8975a1 100644 --- a/modules/settings/package.json +++ b/modules/settings/package.json @@ -12,5 +12,7 @@ "devDependencies": { "@simple-module-py/tsconfig": "*" }, - "dependencies": {} + "dependencies": { + "sonner": "^2.0.7" + } } diff --git a/modules/settings/settings/pages/components/ModuleForm.tsx b/modules/settings/settings/pages/components/ModuleForm.tsx index b6e1f6aa..2464c465 100644 --- a/modules/settings/settings/pages/components/ModuleForm.tsx +++ b/modules/settings/settings/pages/components/ModuleForm.tsx @@ -1,5 +1,6 @@ import { router } from '@inertiajs/react'; import { useEffect, useMemo, useState } from 'react'; +import { toast } from 'sonner'; import { FieldInput, type FieldMeta } from './FieldInput'; import { FieldSource } from './FieldSource'; import { TestConnectionButton } from './TestConnectionButton'; @@ -88,6 +89,7 @@ export function ModuleForm({ module: m, testable = false }: Props) { } setErrors(fieldErrs); } else if (resp.ok) { + toast.success('Settings saved'); router.reload({ only: ['modules'] }); } setBusy(false); diff --git a/modules/settings/tests-js/ModuleForm.test.tsx b/modules/settings/tests-js/ModuleForm.test.tsx new file mode 100644 index 00000000..94c73751 --- /dev/null +++ b/modules/settings/tests-js/ModuleForm.test.tsx @@ -0,0 +1,60 @@ +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, describe, expect, test, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + reload: vi.fn(), + success: vi.fn(), +})); + +vi.mock('@inertiajs/react', () => ({ + router: { reload: mocks.reload }, +})); + +vi.mock('sonner', () => ({ + toast: { success: mocks.success }, +})); + +import { ModuleForm, type ModuleView } from '../settings/pages/components/ModuleForm'; + +const moduleView: ModuleView = { + module_name: 'BackgroundTasks', + package: 'background_tasks', + env_prefix: 'SM_BG_TASKS_', + class_name: 'BackgroundTasksSettings', + fields: [ + { + name: 'retention_days', + type: 'int', + value: 14, + default: 14, + description: '', + is_secret: false, + requires_restart: false, + group: null, + env_var: 'SM_BG_TASKS_RETENTION_DAYS', + }, + ], +}; + +afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); +}); + +describe('ModuleForm', () => { + test('shows success feedback after settings are saved', async () => { + const fetchMock = vi.fn().mockResolvedValue({ ok: true, status: 200 }); + vi.stubGlobal('fetch', fetchMock); + render(); + + fireEvent.change(screen.getByLabelText('retention_days'), { target: { value: '30' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => expect(mocks.success).toHaveBeenCalledWith('Settings saved')); + expect(fetchMock).toHaveBeenCalledWith( + '/api/settings/modules/background_tasks', + expect.objectContaining({ body: JSON.stringify({ retention_days: 30 }) }), + ); + expect(mocks.reload).toHaveBeenCalledWith({ only: ['modules'] }); + }); +}); diff --git a/modules/settings/tests/test_module_api.py b/modules/settings/tests/test_module_api.py index 436e9adf..fd1b2bc9 100644 --- a/modules/settings/tests/test_module_api.py +++ b/modules/settings/tests/test_module_api.py @@ -32,6 +32,28 @@ async def test_put_validation_error_surfaces_422(authenticated_client, app): assert "i18n_default_locale" in resp.text +@pytest.mark.asyncio +async def test_put_rejects_and_does_not_persist_unbounded_background_task_values( + authenticated_client, app, db_session +): + from settings.service import SettingService + from settings.store import SettingsStore + + original = app.state.background_tasks.settings + resp = await authenticated_client.put( + "/api/settings/modules/background_tasks", + json={"retention_days": -5, "max_retries": 999_999_999}, + ) + + assert resp.status_code == 422 + assert {error["loc"][-1] for error in resp.json()["detail"]} == { + "retention_days", + "max_retries", + } + assert app.state.background_tasks.settings is original + assert await SettingsStore(SettingService(db_session)).get_overrides("background_tasks") == {} + + @pytest.mark.asyncio async def test_delete_field_resets_to_default(authenticated_client, app): await authenticated_client.put("/api/settings/modules/host", json={"multi_tenant": True}) diff --git a/modules/settings/tsconfig.json b/modules/settings/tsconfig.json index f543be08..3da1f2d6 100644 --- a/modules/settings/tsconfig.json +++ b/modules/settings/tsconfig.json @@ -6,5 +6,5 @@ "@simple-module-py/ui/*": ["../../packages/ui/src/*"] } }, - "include": ["settings/**/*.ts", "settings/**/*.tsx"] + "include": ["settings/**/*.ts", "settings/**/*.tsx", "tests-js/**/*.ts", "tests-js/**/*.tsx"] } diff --git a/modules/users/tests-js/invite-emails.test.tsx b/modules/users/tests-js/invite-emails.test.tsx new file mode 100644 index 00000000..2016df0c --- /dev/null +++ b/modules/users/tests-js/invite-emails.test.tsx @@ -0,0 +1,41 @@ +import '@testing-library/jest-dom/vitest'; +import { render, screen } from '@testing-library/react'; +import { describe, expect, test, vi } from 'vitest'; + +import { InviteFields } from '../users/pages/Users/components/InviteFields'; +import { isPlausibleEmail, parseInviteEmails } from '../users/pages/Users/invite-emails'; + +describe('invite email validation', () => { + test('parses pasted addresses using every supported separator', () => { + expect(parseInviteEmails('one@example.com; two@example.com\nthree@example.com')).toEqual([ + 'one@example.com', + 'two@example.com', + 'three@example.com', + ]); + }); + + test.each([ + ['teammate@example.com', true], + ['not-an-email', false], + ['missing-domain@', false], + ['missing-tld@example', false], + ])('classifies %s', (email, expected) => { + expect(isPlausibleEmail(email)).toBe(expected); + }); + + test('warns about an invalid address before submit', () => { + render( + , + ); + + expect(screen.getByRole('alert')).toHaveTextContent( + 'not-an-email is not a valid email address.', + ); + }); +}); diff --git a/modules/users/tsconfig.json b/modules/users/tsconfig.json index 0b4e77c4..991c4134 100644 --- a/modules/users/tsconfig.json +++ b/modules/users/tsconfig.json @@ -6,5 +6,5 @@ "@simple-module-py/ui/*": ["../../packages/ui/src/*"] } }, - "include": ["users/**/*.ts", "users/**/*.tsx"] + "include": ["users/**/*.ts", "users/**/*.tsx", "tests-js/**/*.ts", "tests-js/**/*.tsx"] } diff --git a/modules/users/users/pages/Users/AddPeople.tsx b/modules/users/users/pages/Users/AddPeople.tsx index 4816d85e..4c9daad2 100644 --- a/modules/users/users/pages/Users/AddPeople.tsx +++ b/modules/users/users/pages/Users/AddPeople.tsx @@ -10,6 +10,7 @@ import { CreateUserFields } from './components/CreateUserFields'; import { InviteFields } from './components/InviteFields'; import { type InviteResult, InviteResults } from './components/InviteResults'; import { type Role, RolePicker } from './components/RolePicker'; +import { isPlausibleEmail, parseInviteEmails } from './invite-emails'; type Mode = 'invite' | 'create'; @@ -57,11 +58,9 @@ function AddPeople() { prev.includes(roleName) ? prev.filter((r) => r !== roleName) : [...prev, roleName], ); - /** Split a pasted block on commas, semicolons, and any whitespace. */ - const parsedEmails = emails - .split(/[\s,;]+/) - .map((value) => value.trim()) - .filter(Boolean); + const parsedEmails = parseInviteEmails(emails); + const invalidEmails = parsedEmails.filter((value) => !isPlausibleEmail(value)); + const validEmailCount = parsedEmails.length - invalidEmails.length; async function submitInvite() { const resp = await fetch('/api/users/admin/invite/bulk', { @@ -124,7 +123,9 @@ function AddPeople() { } const canSubmit = - mode === 'invite' ? parsedEmails.length > 0 : email.trim() !== '' && password !== ''; + mode === 'invite' + ? validEmailCount > 0 && invalidEmails.length === 0 + : email.trim() !== '' && password !== ''; return ( ) : ( @@ -195,7 +197,7 @@ function AddPeople() { {loading ? 'Working…' : mode === 'invite' - ? `Send ${parsedEmails.length || ''} invite${parsedEmails.length === 1 ? '' : 's'}`.trim() + ? `Send ${validEmailCount || ''} invite${validEmailCount === 1 ? '' : 's'}`.trim() : 'Create user'} diff --git a/modules/users/users/pages/Users/components/InviteFields.tsx b/modules/users/users/pages/Users/components/InviteFields.tsx index 508fda28..1a713900 100644 --- a/modules/users/users/pages/Users/components/InviteFields.tsx +++ b/modules/users/users/pages/Users/components/InviteFields.tsx @@ -5,12 +5,19 @@ import { Info } from 'lucide-react'; interface Props { emails: string; onEmailsChange: (value: string) => void; - /** Addresses parsed out of the box so far. */ + /** Valid addresses parsed out of the box so far. */ count: number; + invalidEmails: string[]; mailerDelivers: boolean; } -export function InviteFields({ emails, onEmailsChange, count, mailerDelivers }: Props) { +export function InviteFields({ + emails, + onEmailsChange, + count, + invalidEmails, + mailerDelivers, +}: Props) { return (
-
+
+ {saved && ( +

+ {t(keys.settings.modules_form.saved_toast)} +

+ )} {testable && }