From 8d0104a009e032e4d1484c4fc8b8e26923c3e471 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 17:00:24 +0200 Subject: [PATCH 1/2] fix(user-directory): follow-ups from #501 review (audit feed at startup, empty labels, single-query stats) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../core/simple_module_core/user_directory.py | 6 ++++-- modules/audit_log/audit_log/module.py | 19 +++++++++++-------- modules/keycloak/keycloak/directory.py | 13 ++++++++----- modules/users/users/directory.py | 17 +++++++++++------ 4 files changed, 34 insertions(+), 21 deletions(-) diff --git a/framework/core/simple_module_core/user_directory.py b/framework/core/simple_module_core/user_directory.py index b4b6ceea7..c6f20c224 100644 --- a/framework/core/simple_module_core/user_directory.py +++ b/framework/core/simple_module_core/user_directory.py @@ -166,13 +166,15 @@ async def resolve_user_labels( """Map id -> :attr:`DirectoryUser.label` for every id the directory knows. Empty and ``None`` ids (a row with no recorded user) are skipped; ids the - directory cannot name are absent, and callers show the stored id instead. + directory cannot name (or names with an empty label, e.g. a cached account + with neither a name nor an email) are absent, and callers show the stored + id instead. """ wanted = {uid for uid in ids if uid} if not wanted: return {} found = await directory.get_many(db, wanted) - return {uid: user.label for uid, user in found.items()} + return {uid: user.label for uid, user in found.items() if user.label} __all__ = [ diff --git a/modules/audit_log/audit_log/module.py b/modules/audit_log/audit_log/module.py index f1576211f..d61c8785c 100644 --- a/modules/audit_log/audit_log/module.py +++ b/modules/audit_log/audit_log/module.py @@ -33,14 +33,6 @@ class AuditLogModule(ModuleBase): i18n_audience="admin", ) - def register_settings(self, app: FastAPI) -> None: - """Offer the audit trail as the app's activity feed (GH #501).""" - from simple_module_core.activity_feed import set_activity_feed - - from audit_log.activity import AuditActivityFeed - - set_activity_feed(app, AuditActivityFeed(app)) - def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from audit_log.endpoints.api import router as api from audit_log.endpoints.views import router as views @@ -69,12 +61,23 @@ def register_permissions(self, registry: PermissionRegistry) -> None: registry.add_group(PERM_GROUP, list(ALL_PERMISSIONS)) async def on_startup(self, app: FastAPI) -> None: + from simple_module_core.activity_feed import set_activity_feed + + from audit_log.activity import AuditActivityFeed from audit_log.capture import audit_callback app.state.sm.db.audit_callback = audit_callback + # Offer the audit trail as the app's activity feed (GH #501). Installed + # here, not in ``register_settings``: this module has no settings, and + # overriding that hook without touching ``app.state.audit_log`` raises + # SM012. + set_activity_feed(app, AuditActivityFeed(app)) async def on_shutdown(self, app: FastAPI) -> None: + from simple_module_core.activity_feed import set_activity_feed + app.state.sm.db.audit_callback = None + set_activity_feed(app, None) def locale_dirs(self) -> dict[str, Path]: base = Path(str(importlib.resources.files(__package__) / "locales")) diff --git a/modules/keycloak/keycloak/directory.py b/modules/keycloak/keycloak/directory.py index b7bdd6641..358e17fca 100644 --- a/modules/keycloak/keycloak/directory.py +++ b/modules/keycloak/keycloak/directory.py @@ -96,12 +96,15 @@ def match_ids(self, term: str) -> Any | None: return select(_UuidText(KeycloakUserCache.id)).where(_name_or_email_contains(term)) async def stats(self, db: Any, *, active_window: timedelta) -> DirectoryStats: - count = select(func.count()).select_from(KeycloakUserCache) - total = (await db.execute(count)).scalar_one() cutoff = datetime.now(UTC) - active_window - active = ( - await db.execute(count.where(KeycloakUserCache.last_login_at >= cutoff)) - ).scalar_one() + total, active = ( + await db.execute( + select( + func.count(), + func.count().filter(KeycloakUserCache.last_login_at >= cutoff), + ).select_from(KeycloakUserCache) + ) + ).one() # The cache row is written at first sign-in, not account creation. return DirectoryStats(total=total, active_since=active, created_this_month=None) diff --git a/modules/users/users/directory.py b/modules/users/users/directory.py index 518b983ca..23d1ad4bb 100644 --- a/modules/users/users/directory.py +++ b/modules/users/users/directory.py @@ -101,12 +101,17 @@ async def stats(self, db: Any, *, active_window: timedelta) -> DirectoryStats: now = datetime.now(UTC) # Calendar month, not a rolling 30 days: the tile reads "+6 this month". month_start = now.replace(day=1, hour=0, minute=0, second=0, microsecond=0) - count = select(func.count()).select_from(User) - total = (await db.execute(count)).scalar_one() - active = ( - await db.execute(count.where(User.last_login_at >= now - active_window)) - ).scalar_one() - created = (await db.execute(count.where(User.created_at >= month_start))).scalar_one() + # One scan for all three figures rather than a query each. + row = ( + await db.execute( + select( + func.count(), + func.count().filter(User.last_login_at >= now - active_window), + func.count().filter(User.created_at >= month_start), + ) + ) + ).one() + total, active, created = row return DirectoryStats(total=total, active_since=active, created_this_month=created) From e70c7fc5a007f0a2947f10f13b6884768bc18057 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 17:19:35 +0200 Subject: [PATCH 2/2] Review follow-ups for user directory: keep select_from in users stats, tests, changelog Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 7 +++++++ framework/core/tests/test_core_user_directory.py | 4 ++++ .../audit_log/tests/test_audit_log_activity_feed.py | 10 ++++++++++ modules/users/users/directory.py | 2 +- 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81c525755..1c28b586b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -271,6 +271,13 @@ All notable changes to this project are documented in this file. The format is b `openapi_public` (`SM_OPENAPI_PUBLIC`) is on. The bare `/openapi.json` was previously served, and anonymous, in production. +### Fixed +- User directory follow-ups (#501): `audit_log` installs its activity feed in + `on_startup` (cleared on shutdown) instead of `register_settings`, which + tripped `SM012`; `resolve_user_labels` omits users whose label is empty so + callers show the stored id; the `users` and `keycloak` directories compute + their stats in one query instead of two or three. + ### Deprecated - `settings.registration`, `settings.module_registry` and `settings.hydrate` re-export the framework objects above and emit a `DeprecationWarning`; diff --git a/framework/core/tests/test_core_user_directory.py b/framework/core/tests/test_core_user_directory.py index 5fbce939e..058b13780 100644 --- a/framework/core/tests/test_core_user_directory.py +++ b/framework/core/tests/test_core_user_directory.py @@ -118,6 +118,10 @@ async def test_resolve_labels_uses_the_one_naming_rule(self): assert labels == {"a": "Ada", "b": "b@x"} assert directory.calls == [{"a", "b", "gone"}] + async def test_resolve_labels_drops_empty_labels(self): + directory = _FakeDirectory([DirectoryUser(id="a", email=""), DirectoryUser("b", "b@x")]) + assert await resolve_user_labels(directory, None, ["a", "b"]) == {"b": "b@x"} + async def test_resolve_labels_short_circuits_without_ids(self): directory = _FakeDirectory([]) assert await resolve_user_labels(directory, None, ["", ""]) == {} diff --git a/modules/audit_log/tests/test_audit_log_activity_feed.py b/modules/audit_log/tests/test_audit_log_activity_feed.py index 108f0a1e4..8b4f50648 100644 --- a/modules/audit_log/tests/test_audit_log_activity_feed.py +++ b/modules/audit_log/tests/test_audit_log_activity_feed.py @@ -58,3 +58,13 @@ async def _names(db, ids): admin_view = await feed.recent_for_actor(db, actor, limit=1, permissions=[WILDCARD]) assert len(admin_view) == 1 + + +async def test_feed_is_installed_on_startup_and_cleared_on_shutdown(app): + from audit_log.module import AuditLogModule + + assert isinstance(get_activity_feed(app), AuditActivityFeed) + await AuditLogModule().on_shutdown(app) + assert get_activity_feed(app) is None + await AuditLogModule().on_startup(app) + assert isinstance(get_activity_feed(app), AuditActivityFeed) diff --git a/modules/users/users/directory.py b/modules/users/users/directory.py index 23d1ad4bb..658bc3f94 100644 --- a/modules/users/users/directory.py +++ b/modules/users/users/directory.py @@ -108,7 +108,7 @@ async def stats(self, db: Any, *, active_window: timedelta) -> DirectoryStats: func.count(), func.count().filter(User.last_login_at >= now - active_window), func.count().filter(User.created_at >= month_start), - ) + ).select_from(User) ) ).one() total, active, created = row