refactor(users): split into admin/auth_local/oauth feature folders - #153
Conversation
Reorganize the users module intra-package from a type-folder layout
(endpoints/, service.py, oauth.py at top level) to a feature-folder
layout where each sub-feature owns its routes, service, views, and
components.
modules/users/users/
admin/ # service, REST API, Inertia views, React components
auth_local/ # login/register/reset/verify/profile + accept-invite
oauth/ # provider builders + per-provider routes (__init__.py
# re-exports the prior `users.oauth.{X}` surface)
Shared infrastructure stays at top level (deps, manager, backend, models,
contracts, mailer, middleware, bootstrap, pages, locales).
UsersModule.register_routes is now the single point of cross-feature
wiring; no feature imports from a sibling feature in either the API or
views layer. The login page's OAuth-provider list is cached once at boot
on app.state.users.oauth_providers so auth_local/views.py doesn't reach
into the oauth feature.
Framework filesystem conventions are preserved unchanged: pages/ stays
a single tree at the package root (required by
simple_module_hosting.manifest.compute_module_pages) and locales/ stays
in place.
Scope: the users module only. The scaffold templates, framework-conventions
docs, and other modules are deliberately not touched — this is a POC to
validate the layout before propagating it.
Verified locally: make lint, make doctor, full pytest (1010 passed), JS
tests (8 passed), and a live uvicorn+Vite run exercising the refactored
auth_local/api login wrapper, auth_local/views login page, admin/api
list, admin/views Inertia index, and the moved admin/components/*.tsx
imports.
Deploying simple-module-python with
|
| Latest commit: |
f6c2688
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://41ed1be1.simple-module-python.pages.dev |
| Branch Preview URL: | https://explore-feature-folder-struc.simple-module-python.pages.dev |
There was a problem hiding this comment.
Pull request overview
Refactors the users module from a type-folder layout into feature folders (admin/, auth_local/, oauth/) while keeping shared infrastructure at the package root and preserving the public users.oauth import surface via re-exports.
Changes:
- Split routes/services/views/components into
adminandauth_localfeature folders; removed legacyendpoints/views module. - Introduced
oauth/providers.pyto centralize provider enablement + client construction and cached enabled providers onapp.state.users. - Updated module wiring (
UsersModule.register_routes/on_startup), imports, and tests to match the new structure.
Reviewed changes
Copilot reviewed 20 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/users/users/state.py | Adds cached oauth_providers to module state; updates rate limiter import path. |
| modules/users/users/module.py | Rewires route registration to feature routers; initializes cached OAuth provider list at startup. |
| modules/users/users/deps.py | Updates UserService dependency to point at admin.service. |
| modules/users/users/oauth/providers.py | New provider enablement + client factory utilities. |
| modules/users/users/oauth/api.py | Uses new provider factory import path for OAuth route registration. |
| modules/users/users/oauth/init.py | Re-exports legacy users.oauth.* surface for backward compatibility. |
| modules/users/users/auth_local/api.py | Local-auth REST endpoints retained, with registration moved to module wiring. |
| modules/users/users/auth_local/views.py | New Inertia views for local-auth pages, reading cached OAuth providers from app state. |
| modules/users/users/auth_local/rate_limit.py | Moves in-process rate limiting helpers under auth_local. |
| modules/users/users/admin/service.py | New admin UserService implementation under feature folder. |
| modules/users/users/admin/api.py | Admin REST endpoints updated to use admin.service.UserService. |
| modules/users/users/admin/views.py | New Inertia admin views split out from the removed endpoints/views.py. |
| modules/users/users/admin/components/IndexFilters.tsx | Admin UI component moved under feature folder. |
| modules/users/users/admin/components/RolesTab.tsx | Admin UI component moved under feature folder. |
| modules/users/users/admin/components/UserRow.tsx | Admin UI component moved under feature folder. |
| modules/users/users/pages/Users/Index.tsx | Updates imports to the new admin/components location. |
| modules/users/tests/test_views.py | Updates imports for moved view helpers. |
| modules/users/tests/test_user_service.py | Updates imports for moved UserService. |
| modules/users/tests/test_service_admin.py | Updates imports for moved UserService. |
| modules/users/tests/test_rate_limit.py | Updates imports for moved rate limiters. |
| modules/users/tests/test_api_auth.py | Updates imports for moved throughput limiter. |
| docs/testing/fixtures.md | Updates example import path for UserService. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Construct settings here (re-reads env_str-bound fields like OAuth | ||
| # client ids/secrets). Validators have already passed by this point — | ||
| # ``register_settings`` ran first and would have raised on placeholders. |
| settings = UsersSettings() | ||
|
|
||
| api_router.include_router(auth_local_api.router) | ||
| api_router.include_router(admin_router) | ||
| # Throughput-wrap the stock fastapi-users routers; ``require_signup_enabled`` | ||
| # gates /register at request time so ``allow_signup`` is hot-reloadable. | ||
| api_router.include_router( | ||
| fastapi_users.get_reset_password_router(), | ||
| prefix="/auth", | ||
| tags=["users-auth"], | ||
| dependencies=[Depends(auth_local_api.enforce_auth_throughput_limit)], | ||
| ) | ||
| api_router.include_router( | ||
| fastapi_users.get_verify_router(UserRead), | ||
| prefix="/auth", | ||
| tags=["users-auth"], | ||
| dependencies=[Depends(auth_local_api.enforce_auth_throughput_limit)], | ||
| ) | ||
| api_router.include_router( | ||
| fastapi_users.get_register_router(UserRead, UserCreate), | ||
| prefix="/auth", | ||
| tags=["users-auth"], | ||
| dependencies=[ | ||
| Depends(auth_local_api.require_signup_enabled), | ||
| Depends(auth_local_api.enforce_auth_throughput_limit), | ||
| ], | ||
| ) | ||
| register_oauth_routes(api_router, settings) | ||
|
|
Address two issues raised in PR review: 1. `oauth/api.py` captured `settings.login_redirect_url` as a string at route-registration time, then closed over it. `UsersModule.on_startup` mutates `app.state.users.settings.login_redirect_url` (dashboard fallback), and admins can change it at runtime via the settings UI, so OAuth callbacks could redirect to a stale URL. Now the callback reads it from `request.app.state.users.settings` at request time. `_build_provider_router` no longer takes a `login_redirect_url` parameter. 2. The `register_routes` comment claimed constructing `UsersSettings()` "re-reads env_str-bound fields like OAuth client ids/secrets" — this is wrong: `env_str()` is evaluated when `UsersSettings` is defined (class-attribute defaults), so re-instantiating doesn't re-read env. Updated to accurately describe why a fresh instance is acceptable here (only `build_clients` reads it, and only static fields) and to point readers at `app.state.users.settings` for mutable fields. Both issues were pre-existing latent behaviour surfaced by the review of this PR. Tests still pass (1010); doctor clean.
|
Addressed both Copilot review comments in 1a175dd: #1 — misleading "re-reads env_str-bound fields" comment — confirmed correct. #2 — stale Tests still pass (1010); |
Summary
Reorganize the
usersmodule from a type-folder layout (endpoints/,service.py,oauth.pyat top level) to a feature-folder layout where each sub-feature owns its routes, service, views, and components.Shared infrastructure stays at top level (
deps,manager,backend,models/,contracts/,mailer/,middleware,bootstrap,pages/,locales/). Framework filesystem conventions are preserved unchanged:pages/stays a single tree at the package root (required bysimple_module_hosting.manifest.compute_module_pages).Design highlights
UsersModule.register_routesis the single point of cross-feature wiring. No feature imports from a sibling feature in either the API or views layer.app.state.users.oauth_providers, soauth_local/views.pyreads from app state instead of reaching into theoauthfeature. Side benefit: the per-requestenabled_provider_names()call on every login render is gone.git mvsogit log --followandgit blame -Mstill trace the source.users.oauth.{OAuthProvider,build_clients,enabled_provider_names}still importable viaoauth/__init__.pyre-export (used bytests/test_oauth.py). Other internal paths (users.service,users.rate_limit,users.endpoints.views) were updated at all call sites — only 5 test files needed changes.Scope (deliberately small)
This is a POC on the
usersmodule only. The scaffold templates (framework/cli/.../templates/module/),docs/framework-conventions.md,CLAUDE.md's architecture section, and other modules are not touched — they continue to describe and produce the old type-folder layout. If this PR is accepted as the convention, a follow-up should propagate the pattern to the scaffold + docs.Test plan
make lint(ruff format + ruff + ty + biome + tsc × 9 + 300-line cap + hardcoded-strings + metadata + READMEs) — cleanmake doctor(SM001–SM019 diagnostics) — cleanmake test-py— 1010 passedmake test-js— 8 passedmake dev(uvicorn + Vite):POST /api/users/auth/login(wrapper inauth_local/api.py) → 204, setssession+sm_authcookiesGET /api/users/me(auth_local/api.py) → 200, returns adminGET /api/users/admin(admin/api.py, mounted via module.py) → 200, user listGET /users/login(auth_local/views.py) → InertiaUsers/Login,oauth_providerssourced fromapp.state.usersGET /users/admin(admin/views.py) → InertiaUsers/Users/Indexpages/Users/Index.tsx's new../../admin/components/{IndexFilters,RolesTab,UserRow}imports → 200 + valid JS