diff --git a/.env.example b/.env.example index cf42bc1c..e1079fc9 100644 --- a/.env.example +++ b/.env.example @@ -31,6 +31,16 @@ SM_DATABASE_URL=sqlite+aiosqlite:///./app.db # SM_USERS_BOOTSTRAP_USER_EMAIL=user@example.com # SM_USERS_BOOTSTRAP_USER_PASSWORD=changeme +# Locales this install ships (optional). `make doctor` checks the locale files +# on disk against one another either way — SM014/SM015 catch a translation that +# has drifted from the default catalog with no configuration at all. Declaring +# the set here additionally turns on SM013, which reports a module that never +# shipped one of the named locales. Like every SM_ variable it takes precedence +# over the value edited at /admin/settings, so leave it commented unless you +# mean to pin the list for this process. +# SM_I18N_SUPPORTED_LOCALES=["en","es"] +# SM_I18N_DEFAULT_LOCALE=en + # ── Process identity ──────────────────────────────────────────────────── # These describe how this process was launched rather than how the app is # configured, so they stay in the environment. diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index 16831c4c..3dff66a2 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -603,6 +603,35 @@ Do NOT declare `const schema = z.object({ ... t('...') })` at module scope — i - Shared UI strings (`packages/ui/`) live in `packages/ui/locales/`, namespaced `ui.*`. - Both are auto-discovered at boot alongside module contributions. +### Strings the server composes + +Neither lint check can see a sentence built in Python. `scripts/check_untranslated_strings.mjs` parses `.tsx`; an f-string in a service reaches the page already rendered, and no catalog can translate it afterwards. + +So a payload field a page renders verbatim must carry *translated* text, not assembled text. Take the request's `Translator` (`TranslatorDep` in an endpoint, or pass it down) and emit a key plus its interpolation arguments: + +```python +# no +summary = f"Changed {', '.join(fields)} on {label}" +# yes +summary = t.t("users.recent_activity.summary.changed_fields", fields=", ".join(fields), label=label) +``` + +This is the same shape as menus (`MenuItem.label_key` resolved in `MenuRegistry.get_for_user(translate=…)`) and audit rows (`AuditLink.label_key`): a registration states a key, the request resolves it, and an unresolved key falls back to the English literal rather than putting a dotted key on screen. + +### One key, one placeholder + +A value that needs its own markup mid-sentence — a permission name in ``, a count in `` — still belongs to *one* catalog key. Splicing `…_prefix` + value + `…_suffix` hands a translator two fragments and no way to move the value, which is the first thing a different word order needs to do. + +`InterpolatedText` (`@simple-module-py/ui/components/InterpolatedText`) renders the whole sentence from one key and puts the markup at the placeholder: + +```tsx + t(keys.host.error.forbidden_permission, { permission: slot })}> + {requiredPermission} + +``` + +The catalog entry is an ordinary `"Your role doesn't include {permission}. Ask an admin."`. A translation that drops the placeholder still shows the value (appended); one that repeats it keeps every word of the copy. + ### Supported locales Configure via env: @@ -628,3 +657,5 @@ The active locale lands on `request.state.locale`. The `` comp ### Diagnostics App boot runs `I18nDiagnostics` against every module's declared locale dirs. See codes `SM013`–`SM016` in the table above. Warnings are printed in dev; errors fail the boot in production. + +`make doctor` runs the same checks, and it runs them whether or not `SM_I18N_SUPPORTED_LOCALES` is set. With the variable unset each namespace is checked against the locale files it actually ships: SM014/SM015 still hold every translation to the default catalog's key set, while SM013 stays quiet, since a locale nothing promised cannot be missing. Declaring the set turns SM013 back on — that is the check that reports a module which never shipped one of the locales the install claims to support. diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index 020d178e..ffec6b6d 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -7,10 +7,14 @@ Exits with status 1 if any ERROR-level diagnostics are reported. -i18n checks are included when ``SM_I18N_SUPPORTED_LOCALES`` is set in env -(or ``.env``). Host-level ``host/locales/`` and shared ``packages/ui/locales/`` -are picked up relative to the project root (``SM_PROJECT_ROOT``, else the -directory of the discovered ``.env``, else the current working dir). +i18n checks always run. ``SM_I18N_SUPPORTED_LOCALES`` (env or ``.env``) +declares the locale set this install promises, which also enables SM013 for a +namespace that never shipped one; with it unset, each namespace is checked +against the locale files it actually has, so key drift between two shipped +translations is still caught. Host-level ``host/locales/`` and shared +``packages/ui/locales/`` are picked up relative to the project root +(``SM_PROJECT_ROOT``, else the directory of the discovered ``.env``, else the +current working dir). """ from __future__ import annotations @@ -36,8 +40,14 @@ from simple_module_core.exceptions import InvalidModuleError -def _load_i18n_settings_from_env() -> tuple[list[str], str] | tuple[None, None]: - """Return ``(supported_locales, default_locale)`` or ``(None, None)`` if unset. +def _load_i18n_settings_from_env() -> tuple[list[str] | None, str]: + """Return ``(supported_locales, default_locale)``. + + ``supported_locales`` is ``None`` when ``SM_I18N_SUPPORTED_LOCALES`` is + unset or unparseable — the diagnostic then falls back to the locale files + each namespace ships rather than running no locale checks at all. The + default locale always has a value, because a parity comparison needs to + know which side is the source of truth. Reads env vars directly to avoid a dependency on ``simple_module_hosting``. Honors ``.env`` by merging it into ``os.environ`` if present (pydantic- @@ -46,9 +56,10 @@ def _load_i18n_settings_from_env() -> tuple[list[str], str] | tuple[None, None]: for key, value in parse_dotenv().items(): os.environ.setdefault(key, value) + default = os.environ.get("SM_I18N_DEFAULT_LOCALE", "en") supported_raw = os.environ.get("SM_I18N_SUPPORTED_LOCALES") if not supported_raw: - return None, None + return None, default try: supported = json.loads(supported_raw) @@ -57,9 +68,8 @@ def _load_i18n_settings_from_env() -> tuple[list[str], str] | tuple[None, None]: supported = [s.strip() for s in supported_raw.split(",") if s.strip()] if not isinstance(supported, list) or not supported: - return None, None + return None, default - default = os.environ.get("SM_I18N_DEFAULT_LOCALE", "en") return supported, default diff --git a/framework/core/simple_module_core/diagnostics/_i18n.py b/framework/core/simple_module_core/diagnostics/_i18n.py index a62b0f71..749724b0 100644 --- a/framework/core/simple_module_core/diagnostics/_i18n.py +++ b/framework/core/simple_module_core/diagnostics/_i18n.py @@ -25,19 +25,28 @@ class I18nDiagnostics: def __init__( self, - supported_locales: list[str], + supported_locales: list[str] | None, default_locale: str, extra_sources: list[tuple[str, str, Path]] | None = None, ) -> None: """Build the diagnostic. + ``supported_locales`` is the set of locales this install promises to + ship. ``None`` means *nobody declared one* — each namespace is then + checked against the locale files it actually has on disk. SM013 is + skipped in that mode (a locale nothing promised cannot be missing), + but SM014/SM015 still hold every shipped translation to the default + locale's key set. Without this, an install that never set + ``SM_I18N_SUPPORTED_LOCALES`` ran no locale checks at all and drift + accumulated with nothing to flag it. + ``extra_sources`` is an optional list of ``(reporter_name, namespace, locale_dir)`` triples for locale directories that aren't owned by any ``ModuleBase`` instance — notably the host's ``host/locales/`` and the shared ``packages/ui/locales/``. ``reporter_name`` is used as the ``module_name`` field on findings for display purposes. """ - self.supported_locales = list(supported_locales) + self.supported_locales = None if supported_locales is None else list(supported_locales) self.default_locale = default_locale self.extra_sources = list(extra_sources or []) @@ -55,10 +64,14 @@ def _check_namespace( ) -> list[Diagnostic]: findings: list[Diagnostic] = [] per_locale_keys: dict[str, set[str]] = {} + declared = self.supported_locales is not None + locales = self.supported_locales if declared else self._locales_on_disk(locale_dir) - for locale in self.supported_locales: + for locale in locales or (): path = locale_dir / f"{locale}.json" if not path.is_file(): + if not declared: + continue findings.append( Diagnostic( level=DiagnosticLevel.WARNING, @@ -119,3 +132,14 @@ def _check_namespace( ) ) return findings + + @staticmethod + def _locales_on_disk(locale_dir: Path) -> list[str]: + """Locale tags a directory actually ships, from its ``.json`` files. + + The default locale sorts first only by accident of the alphabet, which + does not matter: the parity comparison below looks it up by name. + """ + if not locale_dir.is_dir(): + return [] + return sorted(path.stem for path in locale_dir.glob("*.json")) diff --git a/framework/core/simple_module_core/diagnostics/_runner.py b/framework/core/simple_module_core/diagnostics/_runner.py index 582b68fb..fb1d6f6d 100644 --- a/framework/core/simple_module_core/diagnostics/_runner.py +++ b/framework/core/simple_module_core/diagnostics/_runner.py @@ -30,18 +30,20 @@ def run_diagnostics( """Convenience function to run all diagnostics. When ``migration_state`` is provided, also runs migration diagnostics. - When ``i18n_supported_locales`` and ``i18n_default_locale`` are provided, - also runs i18n locale coverage diagnostics. ``i18n_extra_sources`` lets - callers include host/ui locale dirs that aren't owned by a ``ModuleBase``. + ``i18n_default_locale`` turns on the locale checks; ``i18n_supported_locales`` + is the declared locale set, and leaving it empty runs those checks against + whatever locale files are on disk instead of skipping them entirely (see + :class:`I18nDiagnostics`). ``i18n_extra_sources`` lets callers include + host/ui locale dirs that aren't owned by a ``ModuleBase``. """ diagnostics = ModuleDiagnostics().run(modules) - if i18n_supported_locales and i18n_default_locale: + if i18n_default_locale: from simple_module_core.diagnostics._i18n import I18nDiagnostics diagnostics.extend( I18nDiagnostics( - supported_locales=i18n_supported_locales, + supported_locales=i18n_supported_locales or None, default_locale=i18n_default_locale, extra_sources=i18n_extra_sources, ).run(modules) diff --git a/framework/core/tests/test_i18n_diagnostics.py b/framework/core/tests/test_i18n_diagnostics.py index 09b6d323..d53ae9cf 100644 --- a/framework/core/tests/test_i18n_diagnostics.py +++ b/framework/core/tests/test_i18n_diagnostics.py @@ -99,3 +99,43 @@ def test_extra_sources_detect_key_drift(tmp_path: Path) -> None: ).run([]) codes = {f.code for f in findings} assert "SM014" in codes + + +class TestUndeclaredLocaleSet: + """``supported_locales=None`` — nobody said which locales this install ships. + + ``make doctor`` runs with no ``SM_I18N_SUPPORTED_LOCALES`` on a stock + checkout, and the whole diagnostic used to be skipped in that state: drift + between two catalogs that *are* both shipped went unreported because no + third file declared them. The files on disk are themselves the declaration. + """ + + def test_key_drift_is_reported_without_a_declared_locale_set(self, tmp_path: Path) -> None: + _write(tmp_path / "p", "en", {"a": "1"}) + _write(tmp_path / "p", "es", {"a": "1", "orphan": "x"}) + mod = _FakeModule("P", {"p": tmp_path / "p"}) + findings = I18nDiagnostics(supported_locales=None, default_locale="en").run([mod]) + assert {f.code for f in findings} == {"SM015"} + + def test_a_locale_nobody_promised_is_not_reported_missing(self, tmp_path: Path) -> None: + """SM013 is a broken promise; with no declared set there is no promise.""" + _write(tmp_path / "p", "en", {"a": "1"}) + _write(tmp_path / "q", "en", {"a": "1"}) + _write(tmp_path / "q", "es", {"a": "1"}) + mod = _FakeModule("P", {"p": tmp_path / "p", "q": tmp_path / "q"}) + findings = I18nDiagnostics(supported_locales=None, default_locale="en").run([mod]) + assert findings == [] + + def test_extra_sources_are_auto_detected_too(self, tmp_path: Path) -> None: + _write(tmp_path / "ui_locales", "en", {"a": "1", "b": "2"}) + _write(tmp_path / "ui_locales", "es", {"a": "1"}) + findings = I18nDiagnostics( + supported_locales=None, + default_locale="en", + extra_sources=[("packages/ui", "ui", tmp_path / "ui_locales")], + ).run([]) + assert [f.code for f in findings] == ["SM014"] + + def test_a_missing_locale_dir_is_silently_skipped(self, tmp_path: Path) -> None: + mod = _FakeModule("P", {"p": tmp_path / "nope"}) + assert I18nDiagnostics(supported_locales=None, default_locale="en").run([mod]) == [] diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index bdf40fb8..c773adb5 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -2,6 +2,7 @@ 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 { ErrorScreen } from '@simple-module-py/ui/components/ErrorScreen'; +import { InterpolatedText } from '@simple-module-py/ui/components/InterpolatedText'; import { Button } from '@simple-module-py/ui/components/ui/button'; import type { SharedProps } from '@simple-module-py/ui/types'; import type { ReactNode } from 'react'; @@ -125,13 +126,13 @@ function ErrorPage({ let description: ReactNode = message || copy.description; if (required_permission) { description = ( - <> - {t(keys.host.error.forbidden_permission_prefix)}{' '} + t(keys.host.error.forbidden_permission, { permission: slot })} + > {required_permission} - {t(keys.host.error.forbidden_permission_suffix)} - + ); } diff --git a/host/locales/en.json b/host/locales/en.json index 157942d7..18c36e24 100644 --- a/host/locales/en.json +++ b/host/locales/en.json @@ -8,8 +8,7 @@ "error": { "correlation_id_copy": "Copy reference ID", "forbidden_description": "Your role doesn't include the permission this page needs. Ask an admin to grant it.", - "forbidden_permission_prefix": "Your role doesn't include", - "forbidden_permission_suffix": ". Ask an admin to grant it.", + "forbidden_permission": "Your role doesn't include {permission}. Ask an admin to grant it.", "forbidden_title": "No access", "generic_description": "An unexpected error occurred.", "generic_title": "Error", diff --git a/host/locales/es.json b/host/locales/es.json index 5a2ee23e..1769b498 100644 --- a/host/locales/es.json +++ b/host/locales/es.json @@ -8,8 +8,7 @@ "error": { "correlation_id_copy": "Copiar ID de referencia", "forbidden_description": "Tu rol no incluye el permiso que necesita esta página. Pide a un administrador que te lo conceda.", - "forbidden_permission_prefix": "Tu rol no incluye", - "forbidden_permission_suffix": ". Pide a un administrador que te lo conceda.", + "forbidden_permission": "Tu rol no incluye {permission}. Pide a un administrador que te lo conceda.", "forbidden_title": "Sin acceso", "generic_description": "Ocurrió un error inesperado.", "generic_title": "Error", @@ -82,40 +81,40 @@ "restored": "Conexión restablecida" }, "setup": { - "title": "Set up your install", - "subtitle": "A few things before this application is ready to use.", + "title": "Configura tu instalación", + "subtitle": "Unas cuantas cosas antes de que esta aplicación esté lista para usarse.", "connections": { - "heading": "Connections", - "description": "Checking the services this install depends on.", - "retest": "Test again", - "testing": "Testing…" + "heading": "Conexiones", + "description": "Comprobando los servicios de los que depende esta instalación.", + "retest": "Probar de nuevo", + "testing": "Probando…" }, "migrations": { - "heading": "Database schema", - "behind": "The database is behind the version this code expects.", - "current": "The database schema is up to date.", - "apply": "Apply migrations", - "applying": "Applying…" + "heading": "Esquema de la base de datos", + "behind": "La base de datos está por detrás de la versión que espera este código.", + "current": "El esquema de la base de datos está actualizado.", + "apply": "Aplicar migraciones", + "applying": "Aplicando…" }, "administrator": { - "heading": "Create an administrator", - "description": "This account can sign in and manage the install.", - "email": "Email", - "password": "Password", - "full_name": "Full name", - "submit": "Create administrator", - "submitting": "Creating…", - "created": "Administrator created. Reloading…" + "heading": "Crea un administrador", + "description": "Esta cuenta puede iniciar sesión y administrar la instalación.", + "email": "Correo electrónico", + "password": "Contraseña", + "full_name": "Nombre completo", + "submit": "Crear administrador", + "submitting": "Creando…", + "created": "Administrador creado. Recargando…" }, "steps": { - "heading": "Setup steps", - "complete": "Done", - "pending": "Pending", + "heading": "Pasos de configuración", + "complete": "Hecho", + "pending": "Pendiente", "migrations": { - "title": "Apply database migrations", - "description": "Bring the database schema up to the version this code expects." + "title": "Aplicar las migraciones de la base de datos", + "description": "Actualiza el esquema de la base de datos a la versión que espera este código." } }, - "error": "Something went wrong. See the detail above." + "error": "Algo ha salido mal. Consulta el detalle de arriba." } } diff --git a/modules/feature_flags/feature_flags/pages/components/ScopeCard.tsx b/modules/feature_flags/feature_flags/pages/components/ScopeCard.tsx index 57c723d9..2b5a1d1a 100644 --- a/modules/feature_flags/feature_flags/pages/components/ScopeCard.tsx +++ b/modules/feature_flags/feature_flags/pages/components/ScopeCard.tsx @@ -1,5 +1,6 @@ import { Link } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; +import { InterpolatedText } from '@simple-module-py/ui/components/InterpolatedText'; import { Card } from '@simple-module-py/ui/components/ui/card'; import { TenantPicker } from './TenantPicker'; @@ -11,21 +12,12 @@ interface Props { onSelect: (tenantId: string | null) => void; } -// Word Joiner: a zero-width character that cannot appear in real copy, used to -// find where the translator put the tenant id so it can be rendered in the mono -// face. Interpolating twice and splitting keeps word order the translator's -// choice, which pinning the id to the end of the sentence would not. -const SLOT = '\u2060'; - /** * The scope row above the table: which tenant is being looked at, what that * means for unset flags, and a way into the history of every change. */ export function ScopeCard({ tenantId, tenants, auditLogUrl, onSelect }: Props) { const { t } = useT(); - const [hintBefore, hintAfter] = tenantId - ? t(keys.feature_flags.browse.viewing_tenant, { tenant_id: SLOT }).split(SLOT) - : [t(keys.feature_flags.browse.viewing_system), '']; return ( @@ -34,9 +26,18 @@ export function ScopeCard({ tenantId, tenants, auditLogUrl, onSelect }: Props) {

- {hintBefore} - {tenantId && {tenantId}} - {hintAfter} + {tenantId ? ( + // One key, one ordinary `{tenant_id}` placeholder: the translator + // decides where in the sentence the id goes, and it still renders in + // the mono face wherever they put it. + t(keys.feature_flags.browse.viewing_tenant, { tenant_id: slot })} + > + {tenantId} + + ) : ( + t(keys.feature_flags.browse.viewing_system) + )}

{auditLogUrl && ( str: + """``"18 KB"``, ``"840 KB"``, ``"1.2 GB"`` — one decimal, never ``".0"``. + + A 25 MB limit reads as "25 MB"; only a value that genuinely needs the + precision spends a character on it. + """ + for scale, unit in _UNITS: + if count >= scale: + return f"{_round(count / scale)} {unit}" + return f"{count} B" + + +def _round(value: float) -> str: + """One decimal, rounded half *up* and with a bare ``.0`` dropped. + + Half-up rather than :func:`round`, whose banker's rounding turns 1.25 MB + into "1.2 MB" where the TypeScript twin says "1.3 MB". Two numbers for one + limit is exactly the confusion this message exists to remove. + """ + whole, tenth = divmod(math.floor(value * 10 + 0.5), 10) + return str(whole) if tenth == 0 else f"{whole}.{tenth}" diff --git a/modules/file_storage/file_storage/locales/en.json b/modules/file_storage/file_storage/locales/en.json index e71262f7..f716b3ab 100644 --- a/modules/file_storage/file_storage/locales/en.json +++ b/modules/file_storage/file_storage/locales/en.json @@ -54,7 +54,7 @@ }, "errors": { "not_found": "File not found", - "too_large": "File exceeds the maximum allowed size", + "too_large": "File exceeds the {max_size} limit for a single upload", "bad_type": "This file type is not allowed", "backend_error": "Storage backend error" }, diff --git a/modules/file_storage/tests/test_api.py b/modules/file_storage/tests/test_api.py index 940ce759..15608aed 100644 --- a/modules/file_storage/tests/test_api.py +++ b/modules/file_storage/tests/test_api.py @@ -64,3 +64,29 @@ async def test_get_unknown_id_returns_404(authenticated_client: httpx.AsyncClien # /api/* requests without a browser Accept get a JSON {"detail": ...} # body; only browser-shaped requests receive the Inertia error page. assert resp.status_code == 404 + + +async def test_413_names_the_limit_it_enforced(app, authenticated_client: httpx.AsyncClient): + """ "File exceeds the maximum allowed size" tells a rejected uploader nothing. + + The number is the only actionable part of the sentence, and it reached the + client through no other channel on the API path — so it goes in the message + (interpolated by the catalog, not concatenated) and alongside it as raw + bytes for callers that would rather phrase their own copy. + """ + services = app.state.file_storage + original = services.settings.max_file_size_bytes + services.settings.max_file_size_bytes = 4 + try: + resp = await authenticated_client.post( + f"{constants.ROUTE_PREFIX_API}{constants.PATH_UPLOAD}", + files={"file": ("big.txt", b"far too many bytes", "text/plain")}, + ) + finally: + services.settings.max_file_size_bytes = original + + assert resp.status_code == 413, resp.text + detail = resp.json()["detail"] + assert detail["code"] == constants.ErrorCode.TOO_LARGE + assert detail["max_bytes"] == 4 + assert "4 B" in detail["message"] diff --git a/modules/file_storage/tests/test_format.py b/modules/file_storage/tests/test_format.py new file mode 100644 index 00000000..6a43cb42 --- /dev/null +++ b/modules/file_storage/tests/test_format.py @@ -0,0 +1,37 @@ +"""``format_bytes`` must read the same as its TypeScript twin. + +The drop zone says "max 100 MB" from ``pages/format.ts`` and a rejected upload +says the limit from ``format.py``. Two spellings of one number in two places on +one screen is worse than not naming it at all, so the cases below are the ones +``tests-js/format.test.ts`` asserts, transcribed. +""" + +from __future__ import annotations + +import pytest +from file_storage.format import format_bytes + +KIB = 1024 +MIB = KIB * KIB +GIB = MIB * KIB + + +@pytest.mark.parametrize( + ("count", "expected"), + [ + (0, "0 B"), + (1023, "1023 B"), + (KIB, "1 KB"), + (MIB - 1, "1024 KB"), + (MIB, "1 MB"), + (100 * MIB, "100 MB"), + (round(1.2 * GIB), "1.2 GB"), + ], +) +def test_matches_the_typescript_twin(count: int, expected: str) -> None: + assert format_bytes(count) == expected + + +def test_rounds_a_half_away_from_zero() -> None: + """Python's banker's rounding would say "1.2 MB" where the deck says 1.3.""" + assert format_bytes(round(1.25 * MIB)) == "1.3 MB" diff --git a/modules/permissions/permissions/locales/en.json b/modules/permissions/permissions/locales/en.json index f35672cf..ae5306d1 100644 --- a/modules/permissions/permissions/locales/en.json +++ b/modules/permissions/permissions/locales/en.json @@ -46,7 +46,7 @@ "head_title": "Edit Role", "filter_placeholder": "Filter modules or permissions…", "granted_only": "Granted only", - "granted_summary": "of {total} granted", + "granted_summary": "{granted} of {total} granted", "toggle_group_label": "Toggle every {group} permission", "leave_warning": "You have unsaved changes to this role. Leave without saving?" }, diff --git a/modules/permissions/permissions/pages/RoleEdit.tsx b/modules/permissions/permissions/pages/RoleEdit.tsx index c0949a40..ca644ecd 100644 --- a/modules/permissions/permissions/pages/RoleEdit.tsx +++ b/modules/permissions/permissions/pages/RoleEdit.tsx @@ -1,5 +1,6 @@ import { Head, Link, useForm } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; +import { InterpolatedText } from '@simple-module-py/ui/components/InterpolatedText'; 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'; @@ -135,10 +136,20 @@ function RoleEdit({ role, assigned, groups }: Props) {
- {/* The count carries the emphasis, so it is rendered apart from - the sentence rather than interpolated into it. */} - {data.permissions.length}{' '} - {t(keys.permissions.edit.granted_summary, { total: totalRegistered })} + {/* The count carries the emphasis, so it renders in its own + element — but the sentence stays one key with a `{granted}` + placeholder the translator can move, rather than a fragment + that only works with the number in front of it. */} + + t(keys.permissions.edit.granted_summary, { + granted: slot, + total: totalRegistered, + }) + } + > + {data.permissions.length} +
diff --git a/modules/permissions/tests-js/RoleEdit.test.tsx b/modules/permissions/tests-js/RoleEdit.test.tsx index d3397982..6dd46f68 100644 --- a/modules/permissions/tests-js/RoleEdit.test.tsx +++ b/modules/permissions/tests-js/RoleEdit.test.tsx @@ -23,7 +23,7 @@ configureI18n({ 'permissions.edit.cancel_link': 'Cancel', 'permissions.edit.filter_placeholder': 'Filter modules or permissions…', 'permissions.edit.granted_only': 'Granted only', - 'permissions.edit.granted_summary': 'of {total} granted', + 'permissions.edit.granted_summary': '{granted} of {total} granted', 'permissions.edit.toggle_group_label': 'Toggle every {group} permission', }, }); diff --git a/modules/users/tests/test_activity_summary.py b/modules/users/tests/test_activity_summary.py new file mode 100644 index 00000000..eb7ef790 --- /dev/null +++ b/modules/users/tests/test_activity_summary.py @@ -0,0 +1,111 @@ +"""Every activity line comes out of a catalog, not out of an f-string. + +The card used to build its sentences in Python — ``f"{verb} {kind} {label}"`` +from an English verb table — so the "Recent activity" list read as English in +every locale and no translation could reach it. Neither lint check could see +it: they only parse ``.tsx``. + +These tests translate with a stub that echoes ``key(args)`` instead of copy, so +an assertion here fails the moment a sentence is assembled locally again — an +f-string would produce prose where the stub produces a key. +""" + +from __future__ import annotations + +from typing import Any + +from simple_module_core import AuditLink, AuditLinkRegistry +from users.admin.recent_activity import _kind_of, _summarise + + +class _EchoTranslator: + """Stands in for ``Translator``, rendering the key and its arguments.""" + + def t(self, key: str, **params: Any) -> str: + rendered = ", ".join(f"{name}={value}" for name, value in sorted(params.items())) + return f"{key}({rendered})" + + +class _KeylessTranslator(_EchoTranslator): + """A translator with an empty catalog: ``t`` echoes the key back.""" + + def t(self, key: str, **params: Any) -> str: + return key + + +T = _EchoTranslator() + + +class TestSummaryKeys: + def test_a_create_reads_from_the_created_key(self) -> None: + assert _summarise(T, "create", "setting", "smtp_host", []) == ( + "users.recent_activity.summary.created(kind=setting, label=smtp_host)" + ) + + def test_every_spelling_of_one_action_shares_a_key(self) -> None: + """Writers disagree on "delete"/"deleted"; the reader should not.""" + assert _summarise(T, "delete", "file", "x", []) == _summarise(T, "deleted", "file", "x", []) + + def test_an_unknown_action_keeps_its_verb_as_an_argument(self) -> None: + """Inventing a translation for a verb only one writer uses is a guess.""" + assert _summarise(T, "archive", "file", "x", []) == ( + "users.recent_activity.summary.other(action=Archive, kind=file, label=x)" + ) + + def test_named_fields_are_one_argument_not_a_locally_joined_sentence(self) -> None: + summary = _summarise(T, "update", "user", "Ada", [{"field": "email"}, {"field": "roles"}]) + assert summary == ( + "users.recent_activity.summary.changed_fields(fields=email, roles, label=Ada)" + ) + + def test_a_long_field_list_becomes_a_count(self) -> None: + changes = [{"field": name} for name in ("a", "b", "c", "d")] + assert _summarise(T, "update", "user", "Ada", changes) == ( + "users.recent_activity.summary.changed_count(count=4, label=Ada)" + ) + + def test_an_update_with_no_recorded_fields_falls_back_to_the_plain_verb(self) -> None: + assert _summarise(T, "update", "user", "Ada", None) == ( + "users.recent_activity.summary.updated(kind=user, label=Ada)" + ) + + def test_a_missing_catalog_entry_leaves_no_trailing_space(self) -> None: + """The key echoes back verbatim; nothing here appends a stray gap.""" + assert _summarise(_KeylessTranslator(), "create", "setting", "x", []) == ( + "users.recent_activity.summary.created" + ) + + +class TestKindOf: + def _registry(self, link: AuditLink) -> AuditLinkRegistry: + registry = AuditLinkRegistry() + registry.register(link) + return registry + + def test_the_links_label_key_is_translated(self) -> None: + registry = self._registry( + AuditLink( + entity_type="Setting", + url_template="/s/{id}", + label="Setting", + label_key="settings.audit.setting", + ) + ) + assert _kind_of(T, registry, "Setting") == "settings.audit.setting()" + + def test_an_unresolved_key_falls_back_to_the_english_label(self) -> None: + registry = self._registry( + AuditLink( + entity_type="Setting", + url_template="/s/{id}", + label="Setting", + label_key="settings.audit.setting", + ) + ) + assert _kind_of(_KeylessTranslator(), registry, "Setting") == "setting" + + def test_a_type_no_module_claims_reads_as_its_class_name(self) -> None: + assert _kind_of(T, AuditLinkRegistry(), "Unclaimed") == "unclaimed" + + def test_no_registry_at_all_is_survivable(self) -> None: + assert _kind_of(T, None, "Unclaimed") == "unclaimed" diff --git a/modules/users/users/admin/recent_activity.py b/modules/users/users/admin/recent_activity.py index 9770d35e..c853f010 100644 --- a/modules/users/users/admin/recent_activity.py +++ b/modules/users/users/admin/recent_activity.py @@ -7,6 +7,12 @@ ``try/except ImportError`` and the ``None`` return, which the page renders as "no card" rather than "no activity": an install that records nothing and a person who did nothing are different claims. + +Every line is a *translated* sentence, not an assembled one. The rows are +rendered server-side, so this reaches for the request's ``Translator`` the way +menus and audit rows do: a catalog key plus its interpolation arguments. An +f-string here would put English on the card in every locale, and no catalog +could ever reach it. """ from __future__ import annotations @@ -17,6 +23,7 @@ from typing import Any from fastapi import Request +from simple_module_core.i18n import Translator from simple_module_hosting.permissions import resolved_permissions_for from sqlalchemy.ext.asyncio import AsyncSession @@ -28,15 +35,23 @@ AUDIT_LOG_URL = "/admin/audit-log/" -_ACTION_VERBS = { - "create": "Created", - "created": "Created", - "insert": "Created", - "update": "Updated", - "updated": "Updated", - "delete": "Deleted", - "deleted": "Deleted", +_SUMMARY_KEY = "users.recent_activity.summary" + +_ACTION_KEYS = { + "create": "created", + "created": "created", + "insert": "created", + "update": "updated", + "updated": "updated", + "delete": "deleted", + "deleted": "deleted", } +"""Audit ``action`` values to the catalog key that phrases them. + +Several writers spell the same event differently; the card should not. Anything +absent falls through to ``summary.other``, which keeps the raw verb — inventing +a translation for a word only that writer uses would be a guess. +""" _MAX_NAMED_FIELDS = 2 """Past this the field list stops being a summary and becomes the diff.""" @@ -63,8 +78,8 @@ def _changed_fields(changes: Any) -> list[str]: return names -def _summarise(action: str, kind: str, label: str, changes: Any) -> str: - """One line naming what happened, to what. +def _summarise(t: Translator, action: str, kind: str, label: str, changes: Any) -> str: + """One line naming what happened, to what — from the catalog, not an f-string. Field names lead when there are one or two of them, because "changed is_active" is the answer and "updated a User" is not. Beyond that the count @@ -74,24 +89,47 @@ def _summarise(action: str, kind: str, label: str, changes: Any) -> str: "file"), not the model class lowercased: "Created storedfile 6b03…" names a Python class at a reader who is looking at a screen full of files. """ - verb = _ACTION_VERBS.get(action.lower(), action.capitalize()) + action_key = _ACTION_KEYS.get(action.lower()) fields = _changed_fields(changes) - if verb == "Updated" and fields: + if action_key == "updated" and fields: if len(fields) <= _MAX_NAMED_FIELDS: - return f"Changed {', '.join(fields)} on {label}" - return f"Changed {len(fields)} fields on {label}" - return f"{verb} {kind} {label}".rstrip() - - -def _kind_of(registry: Any, entity_type: str) -> str: + # The separator is the catalog's problem too, hence one argument + # rather than one per field: a locale that joins with "، " has + # nowhere to say so if this file does the joining. + return t.t( + f"{_SUMMARY_KEY}.changed_fields", fields=", ".join(fields), label=label + ).strip() + return t.t(f"{_SUMMARY_KEY}.changed_count", count=len(fields), label=label).strip() + if action_key is None: + return t.t( + f"{_SUMMARY_KEY}.other", action=action.capitalize(), kind=kind, label=label + ).strip() + return t.t(f"{_SUMMARY_KEY}.{action_key}", kind=kind, label=label).strip() + + +def _kind_of(t: Translator, registry: Any, entity_type: str) -> str: """What to call this sort of row in a sentence — "setting", "file", "user". Taken from the owning module's audit link, which already states it for the - audit table's type tag. Falling back to the class name keeps a module that - registered no link readable rather than blank. + audit table's type tag, and translated through the link's ``label_key`` + exactly as the audit table does. Falling back to the class name keeps a + module that registered no link readable rather than blank. + + Lowercased because the label is written for a column header ("Setting") + and this is mid-sentence. That is right for English and Spanish and wrong + for German, where a locale that capitalises its nouns should phrase the + whole ``summary.*`` clause around the kind instead of relying on the case + of one interpolated word. """ link = registry.get(entity_type) if registry is not None else None - return ((link.label if link is not None else "") or entity_type).lower() + if link is None: + return entity_type.lower() + label = link.label or entity_type + if link.label_key: + translated = t.t(link.label_key) + if translated != link.label_key: + label = translated + return label.lower() async def _labels_for( @@ -122,6 +160,7 @@ async def recent_activity_for( request: Request, user_id: uuid.UUID, db: AsyncSession, + t: Translator, ) -> list[dict[str, str]] | None: """Recent audit entries where *user_id* was the actor, or ``None``. @@ -157,8 +196,9 @@ async def recent_activity_for( { "at": entry.created_at.isoformat(), "summary": _summarise( + t, entry.action, - _kind_of(registry, entry.entity_type), + _kind_of(t, registry, entry.entity_type), labels.get( (entry.entity_type, entry.entity_id), entry.entity_id[:_SHORT_ID_CHARS], diff --git a/modules/users/users/admin/views.py b/modules/users/users/admin/views.py index d9e64544..b8b0400c 100644 --- a/modules/users/users/admin/views.py +++ b/modules/users/users/admin/views.py @@ -8,6 +8,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse from simple_module_db.deps import get_db +from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.inertia_deps import InertiaDep from simple_module_hosting.permissions import RequiresPermission from sqlalchemy.ext.asyncio import AsyncSession @@ -169,6 +170,7 @@ async def admin_edit_page( user_id: str, request: Request, inertia: InertiaDep, + t: TranslatorDep, service: UserService = Depends(get_user_service), db: AsyncSession = Depends(get_db), ) -> InertiaResponse: @@ -190,6 +192,6 @@ async def admin_edit_page( # ``None`` when the audit_log module is not installed — the card # is then absent rather than empty, which is the honest rendering # of "this deployment does not record activity". - "recent_activity": await recent_activity_for(request, uid, db), + "recent_activity": await recent_activity_for(request, uid, db, t), }, ) diff --git a/modules/users/users/locales/en.json b/modules/users/users/locales/en.json index a5531865..69857e3a 100644 --- a/modules/users/users/locales/en.json +++ b/modules/users/users/locales/en.json @@ -287,7 +287,16 @@ "recent_activity": { "title": "Recent activity", "see_all": "See all in the audit log →", - "empty": "Nothing recorded for this account yet." + "empty": "Nothing recorded for this account yet.", + "summary": { + "created": "Created {kind} {label}", + "updated": "Updated {kind} {label}", + "deleted": "Deleted {kind} {label}", + "other": "{action} {kind} {label}", + "changed_fields": "Changed {fields} on {label}", + "changed_count_one": "Changed {count} field on {label}", + "changed_count_other": "Changed {count} fields on {label}" + } }, "register": { "head_title": "Create your account", diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 7d374ee3..1d630273 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -407,8 +407,7 @@ export default { 'host.admin.title': '', 'host.error.correlation_id_copy': '', 'host.error.forbidden_description': '', - 'host.error.forbidden_permission_prefix': '', - 'host.error.forbidden_permission_suffix': '', + 'host.error.forbidden_permission': '', 'host.error.forbidden_title': '', 'host.error.generic_description': '', 'host.error.generic_title': '', @@ -722,9 +721,13 @@ export default { 'ui.relative_time.just_now': '', 'ui.relative_time.minutes_ago': '', 'ui.relative_time.months_ago': '', + 'ui.relative_time.months_ago_one': '', + 'ui.relative_time.months_ago_other': '', 'ui.relative_time.seconds_ago': '', 'ui.relative_time.unknown': '', 'ui.relative_time.years_ago': '', + 'ui.relative_time.years_ago_one': '', + 'ui.relative_time.years_ago_other': '', 'ui.sidebar.back': '', 'ui.sidebar.close': '', 'ui.sidebar.open': '', @@ -980,6 +983,13 @@ export default { 'users.profile.toast_updated': '', 'users.recent_activity.empty': '', 'users.recent_activity.see_all': '', + 'users.recent_activity.summary.changed_count_one': '', + 'users.recent_activity.summary.changed_count_other': '', + 'users.recent_activity.summary.changed_fields': '', + 'users.recent_activity.summary.created': '', + 'users.recent_activity.summary.deleted': '', + 'users.recent_activity.summary.other': '', + 'users.recent_activity.summary.updated': '', 'users.recent_activity.title': '', 'users.register.bullet_close_prefix': '', 'users.register.bullet_verification': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index c94f3400..06e38949 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -531,8 +531,7 @@ export const keys = { error: { correlation_id_copy: 'host.error.correlation_id_copy', forbidden_description: 'host.error.forbidden_description', - forbidden_permission_prefix: 'host.error.forbidden_permission_prefix', - forbidden_permission_suffix: 'host.error.forbidden_permission_suffix', + forbidden_permission: 'host.error.forbidden_permission', forbidden_title: 'host.error.forbidden_title', generic_description: 'host.error.generic_description', generic_title: 'host.error.generic_title', @@ -941,9 +940,13 @@ export const keys = { just_now: 'ui.relative_time.just_now', minutes_ago: 'ui.relative_time.minutes_ago', months_ago: 'ui.relative_time.months_ago', + months_ago_one: 'ui.relative_time.months_ago_one', + months_ago_other: 'ui.relative_time.months_ago_other', seconds_ago: 'ui.relative_time.seconds_ago', unknown: 'ui.relative_time.unknown', years_ago: 'ui.relative_time.years_ago', + years_ago_one: 'ui.relative_time.years_ago_one', + years_ago_other: 'ui.relative_time.years_ago_other', }, sidebar: { back: 'ui.sidebar.back', @@ -1247,6 +1250,16 @@ export const keys = { recent_activity: { empty: 'users.recent_activity.empty', see_all: 'users.recent_activity.see_all', + summary: { + changed_count: 'users.recent_activity.summary.changed_count', + changed_count_one: 'users.recent_activity.summary.changed_count_one', + changed_count_other: 'users.recent_activity.summary.changed_count_other', + changed_fields: 'users.recent_activity.summary.changed_fields', + created: 'users.recent_activity.summary.created', + deleted: 'users.recent_activity.summary.deleted', + other: 'users.recent_activity.summary.other', + updated: 'users.recent_activity.summary.updated', + }, title: 'users.recent_activity.title', }, register: { diff --git a/packages/ui/locales/en.json b/packages/ui/locales/en.json index b6cf7ce4..48c8b655 100644 --- a/packages/ui/locales/en.json +++ b/packages/ui/locales/en.json @@ -23,7 +23,11 @@ "hours_ago": "{count}h ago", "days_ago": "{count}d ago", "months_ago": "{count}mo ago", + "months_ago_one": "{count}mo ago", + "months_ago_other": "{count}mo ago", "years_ago": "{count}y ago", + "years_ago_one": "{count}y ago", + "years_ago_other": "{count}y ago", "in_minutes": "in {count}m", "in_hours": "in {count}h", "in_days": "in {count}d", diff --git a/packages/ui/src/components/InterpolatedText.test.tsx b/packages/ui/src/components/InterpolatedText.test.tsx new file mode 100644 index 00000000..05363757 --- /dev/null +++ b/packages/ui/src/components/InterpolatedText.test.tsx @@ -0,0 +1,59 @@ +import { render, screen } from '@testing-library/react'; +import { describe, expect, test } from 'vitest'; + +import { InterpolatedText } from './InterpolatedText'; + +/** Stands in for `t(key, params)` over a catalog entry. */ +const translate = (template: string) => (slot: string) => template.replace('{value}', slot); + +describe('InterpolatedText', () => { + test('renders the value where the placeholder sits', () => { + const { container } = render( + + settings.manage + , + ); + expect(container.textContent).toBe('Your role does not include settings.manage. Ask an admin.'); + expect(screen.getByText('settings.manage').tagName).toBe('CODE'); + }); + + test('the placeholder may lead the sentence', () => { + // The whole point of one key with one placeholder: a translation is free + // to put the value first, which splicing a prefix and a suffix cannot do. + const { container } = render( + + 7 + , + ); + expect(container.textContent).toBe('7 of 42 granted'); + }); + + test('a translation that dropped the placeholder still shows the value', () => { + const { container } = render( + 'Permiso insuficiente.'}> + settings.manage + , + ); + expect(container.textContent).toBe('Permiso insuficiente. settings.manage'); + }); + + test('a repeated placeholder keeps every word of the copy', () => { + // Rendering the value twice would be worse than rendering it once, but + // dropping the tail would delete real translated text. + const { container } = render( + `A${slot}B${slot}C`}> + x + , + ); + expect(container.textContent).toBe('AxBC'); + }); + + test('no sentinel character survives into the DOM', () => { + const { container } = render( + + x + , + ); + expect(container.textContent).not.toContain('\u0000'); + }); +}); diff --git a/packages/ui/src/components/InterpolatedText.tsx b/packages/ui/src/components/InterpolatedText.tsx new file mode 100644 index 00000000..c93e6a3d --- /dev/null +++ b/packages/ui/src/components/InterpolatedText.tsx @@ -0,0 +1,57 @@ +import type { ReactNode } from 'react'; + +/** + * Stands in for the styled value while the catalog string is interpolated. + * + * It never reaches a catalog: it is passed as the *value* of an ordinary + * `{name}` placeholder and split back out here, so the only thing a translator + * ever sees — or has to move — is `{name}`. NUL is the sentinel because no + * sentence in any language contains one, where a zero-width joiner or a + * private-use character can legitimately appear in CJK and Indic copy. + */ +const SLOT = '\u0000'; + +interface Props { + /** + * Produce the finished sentence, passing `slot` as the value of the + * placeholder the markup belongs at: + * `(slot) => t(keys.x.y, { permission: slot })`. + */ + render: (slot: string) => string; + /** The value itself, free to carry its own markup. */ + children: ReactNode; +} + +/** + * One translated sentence with one value rendered as markup inside it. + * + * The alternative — a `prefix` key and a `suffix` key spliced around the value + * — hands a translator two sentence fragments and no way to move the value, + * which is the first thing a language with a different word order needs to do. + * Here the whole sentence stays one key with one ordinary placeholder, and the + * value keeps its own ``/`` styling. + */ +export function InterpolatedText({ render, children }: Props) { + const [before, ...rest] = render(SLOT).split(SLOT); + + // A translation that dropped the placeholder still has to show the value. + // Appending it reads awkwardly, but rendering `before` alone would silently + // delete the one word the sentence exists to name. + if (rest.length === 0) { + return ( + <> + {before} {children} + + ); + } + + // Repeating the placeholder is a mistake too, but the text around each copy + // is real translated copy — keep all of it, and render the value once. + return ( + <> + {before} + {children} + {rest.join('')} + + ); +} diff --git a/packages/ui/src/index.ts b/packages/ui/src/index.ts index 587b243b..6401657b 100644 --- a/packages/ui/src/index.ts +++ b/packages/ui/src/index.ts @@ -2,6 +2,7 @@ export { ConfirmActionDialog } from './components/ConfirmActionDialog'; export { ErrorBoundary } from './components/ErrorBoundary'; export { ErrorScreen } from './components/ErrorScreen'; export { FilterPills } from './components/FilterPills'; +export { InterpolatedText } from './components/InterpolatedText'; export { NavIcon } from './components/NavIcon'; export { OfflineBanner } from './components/OfflineBanner'; export { PageShell } from './components/PageShell';