[codex] Expose inactive admin users#694
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the admin user listing functionality by adding support for filtering users by active status and searching across multiple fields, including email, username, display name, user ID, auth provider, and provider user ID. It also exposes the auth_provider and provider_user_id fields in the admin user responses and includes comprehensive E2E tests. The review feedback highlights a logic error in the organization name filter within list_with_organizations that incorrectly includes users without organizations during searches, and points out a sorting inconsistency where users are ordered by UUID instead of creation date.
| AND ($3::TEXT IS NULL | ||
| OR o.name ILIKE ('%' || $3 || '%') ESCAPE '\' | ||
| OR o.id IS NULL) |
There was a problem hiding this comment.
There is a logic error in the organization name filter. When $3 (the organization search query) is provided (i.e., not NULL), the condition OR o.id IS NULL will still evaluate to TRUE for any user who does not belong to any organization. As a result, searching for a specific organization name will incorrectly return all matching users plus all users who have no organizations at all.
To fix this, remove OR o.id IS NULL. When $3 is NULL, the entire clause is satisfied by $3::TEXT IS NULL, which correctly includes users without organizations.
| AND ($3::TEXT IS NULL | |
| OR o.name ILIKE ('%' || $3 || '%') ESCAPE '\' | |
| OR o.id IS NULL) | |
| AND ($3::TEXT IS NULL | |
| OR o.name ILIKE ('%' || $3 || '%') ESCAPE '\\') |
| AND ($5::TEXT IS NULL | ||
| OR o.name ILIKE ('%' || $5 || '%') ESCAPE '\' | ||
| OR o.id IS NULL) |
There was a problem hiding this comment.
Similarly, the organization name filter here has the same logic error. When $5 is provided, OR o.id IS NULL incorrectly includes all users without organizations in the search results.
Remove OR o.id IS NULL to ensure only users belonging to the searched organization are returned.
| AND ($5::TEXT IS NULL | |
| OR o.name ILIKE ('%' || $5 || '%') ESCAPE '\' | |
| OR o.id IS NULL) | |
| AND ($5::TEXT IS NULL | |
| OR o.name ILIKE ('%' || $5 || '%') ESCAPE '\\') |
| AND ($5::TEXT IS NULL | ||
| OR o.name ILIKE ('%' || $5 || '%') ESCAPE '\' | ||
| OR o.id IS NULL) | ||
| ORDER BY u.id, o.created_at ASC NULLS LAST |
There was a problem hiding this comment.
Using ORDER BY u.id as the primary sort key (required by DISTINCT ON (u.id)) results in users being returned in an arbitrary order (sorted by their UUID). This is inconsistent with list_admin, which returns users sorted by created_at DESC (newest first), making the admin UI behavior unpredictable depending on whether organizations are included.
Consider wrapping the query in a subquery or CTE to apply DISTINCT ON first, and then sort the final paginated result set by created_at DESC on the outer query.
PierreLeGuen
left a comment
There was a problem hiding this comment.
Reviewed the admin user-visibility changes. Logic is sound — inactive users are now included by default, the multi-field search is correctly parameterized and LIKE-escaped (nice dedup into escape_like_query), and the org-path count/list use COUNT(DISTINCT u.id) / DISTINCT ON (u.id) so the new joins don't inflate totals. A few inline notes below; none are blocking.
| Ok((result, total_count)) | ||
| } | ||
|
|
||
| async fn get_active_user_count(&self) -> Result<i64> { |
There was a problem hiding this comment.
get_active_user_count is removed from the AdminRepository trait here, but the underlying inherent method UserRepository::get_active_user_count (crates/database/src/repositories/user.rs:206) now has no callers anywhere in the workspace. Worth deleting it in this PR so it doesn't linger as dead code.
| params.limit, | ||
| params.offset, | ||
| params.include_organizations, | ||
| params.search, |
There was a problem hiding this comment.
Minor privacy note: search is logged here verbatim, and an admin searching by email / display name puts PII into the log line. It's debug! (not emitted at prod info level), so this is low-risk and consistent with the pre-existing search_by_name logging — just flagging given the repo's strict logging rules. Consider logging only search.is_some() if you want to be conservative.
| let count_row = client | ||
| .query_one( | ||
| r#" | ||
| SELECT COUNT(*) as total_count |
There was a problem hiding this comment.
Non-blocking perf note: when search is set this runs a leading-wildcard ILIKE ('%' || $ || '%') across 6 columns, and it's executed twice per request (count + page), neither of which can use a btree index → sequential scan of users. Fine at current admin-tooling scale; if the users table grows large this is the first thing that'll get slow. (When search is NULL the $::TEXT IS NULL guard short-circuits, so the no-search path is unaffected.)
Summary
/v1/admin/users, including theinclude_organizations=truepath.auth_provider,provider_user_id) to admin user responses.searchandis_activefilters with totals based on the exact filtered result set.Why
Admins could not reliably diagnose login failures caused by hidden inactive user rows or OAuth identity collisions without direct production DB access. This makes those rows discoverable from admin tooling.
Validation
cargo fmtcargo check -p apigit diff --checkcargo test -p api --test e2e_all test_admin_list_users_finds_inactive_and_oauth_identity_fields -- --nocapturecompiled, then failed during local test DB bootstrap because Postgres was not accepting connections (Connection refused).