Skip to content

refacto(users): refacto get /v1/admin/users endpoint toward clean architecture#893

Merged
leoguillaume merged 6 commits into
mainfrom
723-refacto-refacto-get-v1adminusers-endpoint-toward-clean-architecture
May 29, 2026
Merged

refacto(users): refacto get /v1/admin/users endpoint toward clean architecture#893
leoguillaume merged 6 commits into
mainfrom
723-refacto-refacto-get-v1adminusers-endpoint-toward-clean-architecture

Conversation

@benjaminpilia
Copy link
Copy Markdown
Contributor

//: # (# Pull Request Title: refacto(clean-architecture): GET /v1/admin/users and GET /v1/admin/users/{user_id} endpoints)

Overview

Closes #723.

Refactors the GET /v1/admin/users and GET /v1/admin/users/{user_id} endpoints toward the clean architecture pattern already applied to other admin endpoints (roles, routers, providers).

What was done:

  • Added GetOneUserUseCase and GetUsersUseCase with their commands and result types
  • Added get_user_by_id to UserRepository (abstract) and PostgresUserRepository (implementation)
  • Refactored get_users in PostgresUserRepository: added sort_by/sort_order parameters, fixed the total count bug (filters were not applied to the count query), returns UserPage instead of a raw list
  • Added UserSortField (StrEnum) and UserPage to the user domain
  • Added UsersResponse schema
  • Extracted the _USER_COLUMNS constant in PostgresUserRepository to eliminate the SELECT column list duplication (�4 queries)
  • Extracted the has_expired property to UserWithRoleView and replaced all inline expiration checks across use cases (�21 files)
  • Updated the playground to use the new query parameter names

Definition of Done:

  • GET /v1/admin/users/{user_id} implemented following clean architecture
  • GET /v1/admin/users implemented following clean architecture (pagination, filtering, sorting)
  • Unit tests for GetOneUserUseCase
  • Unit tests for GetUsersUseCase
  • Integration tests for GET /v1/admin/users/{user_id} endpoint
  • Integration tests for GET /v1/admin/users endpoint
  • Integration tests for PostgresUserRepository.get_user_by_id
  • Integration tests for PostgresUserRepository.get_users

Breaking changes:

  • No breaking changes
  • This PR contains breaking changes (explained below)

GET /v1/admin/users ? query parameter names have changed:

Old parameter New parameter Note
role role_id renamed
organization organization_id renamed
order_by sort_by renamed, now accepts UserSortField enum values
order_direction sort_order renamed, now accepts SortOrder enum values
email (removed) email filtering is no longer supported

The playground has been updated accordingly.

Check lists

Review checklist

  • Updated or added documentation
  • Updated or added unit tests
  • Updated or added integration tests
  • No debug logs or commented-out code left
  • No secrets or environment variables committed in clear text
  • Code is linted and formatted using the project pre-commit hooks

If api/sql/models.py has been modified, please confirm that the following steps have been completed:

  • Alembic migration has been generated
  • Alembic migration upgrade has been tested locally
  • Alembic migration downgrade has been tested locally

Deployment checklist

  • Alembic migration has been generated
  • Configuration file has been modified
  • Environment variables have been modified

Additional Notes

has_expired refactoring scope

The has_expired property was extracted from inline checks scattered across all use cases. This touched 21 files beyond the scope of this issue but was a necessary cleanup ? import time was present and used directly in every use case, which was the pre-existing pattern before UserWithRoleView had the property. All changes are mechanical substitutions with no behavioural impact.

email filter removal

The old GET /v1/admin/users accepted an email query parameter for exact-match filtering. This has been removed because:

  1. The new UserRepository abstraction does not expose email-based filtering for list queries (only get_user_by_email for single-user lookup exists)
  2. Email search was not part of the issue scope

If email filtering is needed in the future, it can be added as a dedicated use case parameter and repository method.

Tech debt deferred to a follow-up task

The admin authentication check (has_expired + is_admin) is still duplicated across all 15 admin use cases. A check_admin_access() method on UserWithRoleView was identified as the correct fix but is out of scope for this PR.

@benjaminpilia benjaminpilia force-pushed the 723-refacto-refacto-get-v1adminusers-endpoint-toward-clean-architecture branch from b49bd9c to 4fa19ed Compare May 28, 2026 14:32
@leoguillaume leoguillaume changed the title 723 refacto refacto get v1adminusers endpoint toward clean architecture refacto(users): refacto get /v1/admin/users endpoint toward clean architecture May 29, 2026


@dataclass
class GetOneUserCommand:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a deux syntaxes pour les use cases de GET

  • get routers / get one router (pour providers, routers + users ?)
  • get models / get model (pour models et roles )

A choisir je préfère l'option 2

@leoguillaume leoguillaume merged commit 8e9cf2d into main May 29, 2026
@leoguillaume leoguillaume deleted the 723-refacto-refacto-get-v1adminusers-endpoint-toward-clean-architecture branch May 29, 2026 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

 [refacto] Refacto GET /v1/admin/users endpoint toward clean architecture

2 participants