[US-2.2 / OBT-201] Language usage stats endpoint - #98
Conversation
Add GET /api/languages/{id}/stats returning project_count so admins can
gauge the impact before deactivating a language. Returns 404 for an
unknown language and requires authentication.
The usage-stats endpoint now returns which projects use a language, not
just the count. GET /api/languages/{id}/stats exposes a `projects` array
of {id, name} alongside `project_count`, so the console can list the
affected projects before deactivating and explain an in-use block.
- LanguageStatsResponse.projects (LanguageProjectRef {id, name})
- get_language_stats returns (id, name) rows ordered by name
- tests assert the returned project names and the empty case
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
📝 WalkthroughWalkthroughAdds language statistics response models, a service query for language-associated projects, an authenticated ChangesLanguage statistics
Sequence Diagram(s)sequenceDiagram
participant Client
participant LanguageStatsEndpoint
participant LanguageStatsService
participant Database
Client->>LanguageStatsEndpoint: GET /{language_id}/stats
LanguageStatsEndpoint->>LanguageStatsService: Request language statistics
LanguageStatsService->>Database: Validate language and query ordered projects
Database-->>LanguageStatsService: Project id and name tuples
LanguageStatsService-->>LanguageStatsEndpoint: Project tuples
LanguageStatsEndpoint-->>Client: LanguageStatsResponse
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
tests/test_language_stats.py-9-18 (1)
9-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert project IDs and ordering explicitly.
The current set-based assertion would pass with incorrect IDs, mismatched ID/name pairs, or without the service’s
order_by(Project.name). Capture the created projects and assert the exact ordered tuples.Suggested test adjustment
- await make_project(db_session, language_id=lang.id, name="P1") - await make_project(db_session, language_id=lang.id, name="P2") + first = await make_project(db_session, language_id=lang.id, name="Zeta") + second = await make_project(db_session, language_id=lang.id, name="Alpha") ... - assert len(projects) == 2 - assert {name for _, name in projects} == {"P1", "P2"} + assert projects == [(second.id, "Alpha"), (first.id, "Zeta")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_language_stats.py` around lines 9 - 18, Update test_language_stats_lists_projects to retain the project objects returned by make_project, then assert the exact ordered (project ID, project name) tuples from get_language_stats. Replace the set-based name assertion while preserving the expected P1, P2 ordering established by the service.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@tests/test_language_stats.py`:
- Around line 9-18: Update test_language_stats_lists_projects to retain the
project objects returned by make_project, then assert the exact ordered (project
ID, project name) tuples from get_language_stats. Replace the set-based name
assertion while preserving the expected P1, P2 ordering established by the
service.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: f68b2016-2044-46f7-aeba-6c63d455b2cd
📒 Files selected for processing (5)
app/api/languages.pyapp/models/language.pyapp/services/language/__init__.pyapp/services/language/get_language_stats.pytests/test_language_stats.py
…tats test Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…ce conventions Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
@coderabbitai — decision on this review:
|
|
Tip For best results, initiate chat on the files or code changes.
(¬‸¬)b |
GET /languages/{id}/stats was guarded by get_current_user and returned the id
and name of every project using the language, with no scoping against the
caller's own projects. Any manager could walk language ids and inventory the
projects of organizations they have no access to, which contradicts the
managed-project scoping US-11.6 establishes for the console lists.
The endpoint exists to inform the deactivation decision, and deactivation is now
platform-admin only (US-2.1), so a manager has no remaining use for it. Guarding
with require_platform_admin closes the exposure outright instead of narrowing it.
The console only ever calls this from the admin-gated deactivation dialog.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
| async def get_language_stats( | ||
| language_id: str, | ||
| db: AsyncSession = Depends(get_db), | ||
| _: User = Depends(require_platform_admin), |
There was a problem hiding this comment.
Is that correct? Relative to main this /stats route is new — it was added on the first commit of this same PR (58d0d63) — so I think nothing was ever exposed, but the body reads like a leak that shipped. And the require_admin_or_manager you mention is not on main, it comes on #93. The admin guard itself is a good call, just please fix the body and the "403 instead of 200" breaking change checkbox — they do not apply for an endpoint that never existed outside this PR.
| from app.services.language.get_language_or_404 import get_language_or_404 | ||
|
|
||
|
|
||
| async def get_language_stats(db: AsyncSession, language_id: str) -> list[tuple[str, str]]: |
There was a problem hiding this comment.
For me it is relevant type the outputs whenever possible. Project.id and Project.name are both str here, so if the select() column order changes one day the router unpacking for project_id, name in projects swaps them silently and mypy will not catch it. app/services/oral_collector/stats_service.py already builds the Pydantic items inside the service and returns GenreStatsResponse directly to the router — please verify that and if it is the majority for stats on the codebase, adapt this one to return the typed model.
| return LanguageStatsResponse( | ||
| language_id=language_id, | ||
| project_count=len(projects), | ||
| projects=[LanguageProjectRef(id=project_id, name=name) for project_id, name in projects], |
There was a problem hiding this comment.
ps: typed outputs issue again — building the LanguageProjectRef list and the count here is model creation on the router, CLAUDE.md asks to keep this on the service.
…e-a-given-language-before
2 participants
Summary
Extends the language usage-stats endpoint so it returns which projects use a language, not just how many.
GET /api/languages/{id}/statsnow includes aprojectsarray of{ id, name }alongsideproject_count. The console deactivate confirmation uses this to list the affected projects before an admin confirms.Also restricts the endpoint to platform admins, closing a cross-organization exposure — see below.
Changes
app/models/language.py: newLanguageProjectRef { id, name };LanguageStatsResponsegainsprojects: list[LanguageProjectRef](keepsproject_count).app/services/language/get_language_stats.py: selects(Project.id, Project.name)for the language, ordered by name.app/api/languages.py: buildsproject_count = len(projects)and theprojectsarray.app/api/languages.py: the route is guarded byrequire_platform_admininstead ofget_current_user.Security fix — cross-organization project exposure
As written, the endpoint was guarded only by
get_current_userand the router-levelrequire_admin_or_manager, both of which admit a manager.get_language_statsrunsselect(Project.id, Project.name).where(Project.language_id == language_id)with no scoping against the caller's own projects. Any manager could therefore walk language ids — which they already hold, sincelanguage_idis a required field on every project response — and inventory theid+nameof projects belonging to organizations they have no access to. That directly contradicts the managed-project scoping US-11.6 establishes for the console lists.Guarded with
require_platform_adminrather than narrowed by scoping, because the endpoint exists to inform the deactivation decision and deactivation is now platform-admin only (US-2.1 / OBT-200, rule revised 2026-07-16). A manager has no remaining use for it. Verified in the console: the only caller isopenDeleteDialoginLanguagesPage, reachable exclusively from the admin-gated deactivate action — so this is not a frontend regression.Type of Change
projectsfield)403instead of200(no such caller exists in the console)Testing
tests/test_language_stats.py: returned ids/names, the empty case, and unknown language →404. The service tests are unaffected by the guard change (the guard lives on the route).ruff check .+ruff format --check .clean.