feat: add bulk visibility toggle for admin players

forgejo_admin commented

2026-03-28 21:20:49 +00:00

Author

Owner

PR #218 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic
Files changed: 2 (204 additions, 0 deletions)

Route ordering -- Verified. @router.patch("/players/bulk-visibility") is defined at line 922, before @router.patch("/players/{player_id}/visibility") at line 976. FastAPI path parameter conflicts are avoided.

Admin auth -- Verified. Uses user: User = Depends(require_admin) where require_admin = require_role("admin") is the module-level dependency shared by all admin endpoints. No DRY violation, no auth bypass path.

Atomic behavior -- Verified. The flow is: (1) query players via IN, (2) compare count to input length, (3) raise 404 if mismatch (before any mutation), (4) mutate and commit only if all players found. The 404 path never touches is_public, so no rollback is needed -- changes simply never happen.

Input validation -- Pydantic enforces player_ids: list[int] and is_public: bool at the model level. Empty list is explicitly guarded with a 400. SQLAlchemy uses parameterized queries for the IN clause. No SQL injection risk.

Pydantic models -- BulkVisibilityRequest and BulkVisibilityResponse are clean, minimal, and correctly typed. Response includes updated count, sorted player_ids, and the applied is_public value. Good.

Logging -- Audit log includes player count, visibility value, and admin email. Appropriate for an admin mutation endpoint.

SQLAlchemy patterns -- .query().filter().all() is standard. Session commit is called once after all mutations. No N+1 issues.

Test coverage (5 tests):

Test	What it covers
`test_bulk_update_three_players`	Happy path: 3 players set to `is_public=True`, verifies response and DB persistence
`test_bulk_update_set_false`	Reverse case: set previously-public players to `is_public=False`
`test_empty_list_returns_400`	Edge case: empty `player_ids` list rejected
`test_nonexistent_player_returns_404_no_changes`	Atomic: mix of real and fake IDs returns 404, verifies existing player unchanged
`test_non_admin_rejected`	Auth: unauthenticated client gets 401/403

Test fixtures are well-structured: admin_client overrides both get_db and require_admin, client (from conftest) overrides only get_db so real auth rejects.

BLOCKERS

None.

NITS

Duplicate player_ids edge case -- If the caller sends player_ids: [1, 1, 1], the SQL IN query returns 1 row, but len(payload.player_ids) is 3. This triggers a misleading 404 with "Players not found: []" (empty missing list since set(payload.player_ids) - found_ids is empty after dedup). Two options: (a) deduplicate payload.player_ids at the top of the function (payload.player_ids = list(set(payload.player_ids))), or (b) add a Pydantic validator that rejects duplicates. Either way, a test for this case would be good. Non-blocking since real callers are unlikely to send duplicates, but the empty-missing-list 404 is confusing if they do.
Consider db.flush() instead of db.commit() for testability -- The current pattern works, but using db.flush() + letting the request lifecycle handle commit would be more consistent with transactional patterns in larger apps. Very minor, not blocking.

SOP COMPLIANCE

Branch named after issue: 217-bulk-visibility follows {issue-number}-{kebab-case-purpose} convention
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
Related references issue: Closes #217 present
Related references plan slug: N/A -- standalone feature, no plan. Acceptable.
No secrets committed
No scope creep -- exactly 2 files changed, both directly related to the feature
Ruff format/lint reported clean

PROCESS OBSERVATIONS

Clean, focused PR. Single endpoint + comprehensive tests. The existing single-player visibility endpoint (/players/{player_id}/visibility) is untouched and its 7 tests are reported as still passing. Route ordering was considered and handled correctly. Good deployment frequency signal -- small, reviewable, mergeable.

VERDICT: APPROVED

## PR #218 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic **Files changed:** 2 (204 additions, 0 deletions) **Route ordering** -- Verified. `@router.patch("/players/bulk-visibility")` is defined at line 922, before `@router.patch("/players/{player_id}/visibility")` at line 976. FastAPI path parameter conflicts are avoided. **Admin auth** -- Verified. Uses `user: User = Depends(require_admin)` where `require_admin = require_role("admin")` is the module-level dependency shared by all admin endpoints. No DRY violation, no auth bypass path. **Atomic behavior** -- Verified. The flow is: (1) query players via `IN`, (2) compare count to input length, (3) raise 404 if mismatch (before any mutation), (4) mutate and commit only if all players found. The 404 path never touches `is_public`, so no rollback is needed -- changes simply never happen. **Input validation** -- Pydantic enforces `player_ids: list[int]` and `is_public: bool` at the model level. Empty list is explicitly guarded with a 400. SQLAlchemy uses parameterized queries for the `IN` clause. No SQL injection risk. **Pydantic models** -- `BulkVisibilityRequest` and `BulkVisibilityResponse` are clean, minimal, and correctly typed. Response includes `updated` count, sorted `player_ids`, and the applied `is_public` value. Good. **Logging** -- Audit log includes player count, visibility value, and admin email. Appropriate for an admin mutation endpoint. **SQLAlchemy patterns** -- `.query().filter().all()` is standard. Session commit is called once after all mutations. No N+1 issues. **Test coverage (5 tests):** | Test | What it covers | |------|---------------| | `test_bulk_update_three_players` | Happy path: 3 players set to `is_public=True`, verifies response and DB persistence | | `test_bulk_update_set_false` | Reverse case: set previously-public players to `is_public=False` | | `test_empty_list_returns_400` | Edge case: empty `player_ids` list rejected | | `test_nonexistent_player_returns_404_no_changes` | Atomic: mix of real and fake IDs returns 404, verifies existing player unchanged | | `test_non_admin_rejected` | Auth: unauthenticated client gets 401/403 | Test fixtures are well-structured: `admin_client` overrides both `get_db` and `require_admin`, `client` (from conftest) overrides only `get_db` so real auth rejects. ### BLOCKERS None. ### NITS 1. **Duplicate player_ids edge case** -- If the caller sends `player_ids: [1, 1, 1]`, the SQL `IN` query returns 1 row, but `len(payload.player_ids)` is 3. This triggers a misleading 404 with `"Players not found: []"` (empty missing list since `set(payload.player_ids) - found_ids` is empty after dedup). Two options: (a) deduplicate `payload.player_ids` at the top of the function (`payload.player_ids = list(set(payload.player_ids))`), or (b) add a Pydantic validator that rejects duplicates. Either way, a test for this case would be good. Non-blocking since real callers are unlikely to send duplicates, but the empty-missing-list 404 is confusing if they do. 2. **Consider `db.flush()` instead of `db.commit()` for testability** -- The current pattern works, but using `db.flush()` + letting the request lifecycle handle commit would be more consistent with transactional patterns in larger apps. Very minor, not blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `217-bulk-visibility` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references issue: `Closes #217` present - [ ] Related references plan slug: N/A -- standalone feature, no plan. Acceptable. - [x] No secrets committed - [x] No scope creep -- exactly 2 files changed, both directly related to the feature - [x] Ruff format/lint reported clean ### PROCESS OBSERVATIONS Clean, focused PR. Single endpoint + comprehensive tests. The existing single-player visibility endpoint (`/players/{player_id}/visibility`) is untouched and its 7 tests are reported as still passing. Route ordering was considered and handled correctly. Good deployment frequency signal -- small, reviewable, mergeable. ### VERDICT: APPROVED

2026-03-28 21:18:20 +00:00

Summary

Adds PATCH /admin/players/bulk-visibility endpoint that accepts a list of player IDs and an is_public flag, bulk-updating visibility in a single atomic operation. Returns 404 with no changes if any player ID is missing.

Changes

src/basketball_api/routes/admin.py — Added BulkVisibilityRequest/BulkVisibilityResponse models and bulk_toggle_visibility endpoint. Placed before /players/{player_id}/visibility to avoid FastAPI path parameter conflicts.
tests/test_bulk_visibility.py — 5 tests covering bulk update (3 players), set-false, empty list (400), nonexistent player (404 atomic), and non-admin rejection (403).

Test Plan

python -m pytest tests/test_bulk_visibility.py -v — all 5 pass
python -m pytest tests/test_player_visibility.py -v — all 7 existing tests still pass (no regressions)
ruff format and ruff check clean

Review Checklist

Tests pass locally
Ruff format and lint clean
Route order verified (bulk-visibility before {player_id}/visibility)
Atomic behavior verified (404 rolls back all changes)
Existing visibility tests unaffected

Forgejo issue: #217

Closes #217

N/A — no pal-e-docs notes for this change.

## Summary Adds `PATCH /admin/players/bulk-visibility` endpoint that accepts a list of player IDs and an `is_public` flag, bulk-updating visibility in a single atomic operation. Returns 404 with no changes if any player ID is missing. ## Changes - `src/basketball_api/routes/admin.py` — Added `BulkVisibilityRequest`/`BulkVisibilityResponse` models and `bulk_toggle_visibility` endpoint. Placed before `/players/{player_id}/visibility` to avoid FastAPI path parameter conflicts. - `tests/test_bulk_visibility.py` — 5 tests covering bulk update (3 players), set-false, empty list (400), nonexistent player (404 atomic), and non-admin rejection (403). ## Test Plan - `python -m pytest tests/test_bulk_visibility.py -v` — all 5 pass - `python -m pytest tests/test_player_visibility.py -v` — all 7 existing tests still pass (no regressions) - `ruff format` and `ruff check` clean ## Review Checklist - [x] Tests pass locally - [x] Ruff format and lint clean - [x] Route order verified (bulk-visibility before {player_id}/visibility) - [x] Atomic behavior verified (404 rolls back all changes) - [x] Existing visibility tests unaffected ## Related - Forgejo issue: #217 Closes #217 ## Related Notes N/A — no pal-e-docs notes for this change.

forgejo_admin added 1 commit

feat: add bulk visibility toggle for admin players

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

0ace27465b

PATCH /admin/players/bulk-visibility accepts player_ids list and
is_public flag. Atomic — returns 404 if any player not found. Requires
admin role.

Closes #217

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Test	What it covers
`test_bulk_update_three_players`	Happy path: 3 players set to `is_public=True`, verifies response and DB persistence
`test_bulk_update_set_false`	Reverse case: set previously-public players to `is_public=False`
`test_empty_list_returns_400`	Edge case: empty `player_ids` list rejected
`test_nonexistent_player_returns_404_no_changes`	Atomic: mix of real and fake IDs returns 404, verifies existing player unchanged
`test_non_admin_rejected`	Auth: unauthenticated client gets 401/403

forgejo_admin merged commit 6c39a7ca04 into main

2026-03-28 21:21:19 +00:00

forgejo_admin deleted branch 217-bulk-visibility

forgejo_admin referenced this pull request from a commit

feat: add bulk visibility toggle for admin players (#218)

Rows
Columns

feat: add bulk visibility toggle for admin players #218