feat: add GET /codes endpoint, enrich CodeResponse with location_name

forgejo_admin commented

2026-03-16 19:59:55 +00:00

Author

Contributor

PR #15 Review

DOMAIN REVIEW

Tech stack: Python 3.14 / FastAPI / SQLAlchemy / Pydantic / Alembic / PostgreSQL

Python/FastAPI/SQLAlchemy checklist:

PEP 484 type hints: All function signatures and return types are properly annotated. CodeStatus(str, Enum) is correctly typed.
PEP 257 docstrings: New endpoint has a docstring explaining filter semantics. Module-level docstring present.
SQLAlchemy patterns: Single JOIN query (CouponUsage JOIN Location) avoids N+1. Explicit column selection in the query avoids loading unnecessary data. Session passed via FastAPI dependency injection -- correct pattern.
FastAPI specifics: Query() parameter with description for the status filter. response_model=list[CodeResponse] correctly declared. Enum-based query parameter gives automatic 422 on invalid values.
OWASP / Input validation: User isolation enforced via CouponUsage.keycloak_sub == user.sub in the JOIN filter. No raw SQL. Enum restricts status values to 3 known strings. No injection risk.
Ruff compliance: # noqa: E712 comments on SQLAlchemy boolean comparisons (== False, == True) are correct -- SQLAlchemy requires this form for SQL generation.

Semantic observation -- "active" definition:

The _count_active_codes() function in locations.py (used for slot enforcement) defines "active" as expires_at > now() only, which includes redeemed codes. The new GET /codes?status=active defines "active" as redeemed == False AND expires_at > now(), which excludes redeemed codes. This is the correct design choice: for slot enforcement, a redeemed code still occupies a slot until expiry; for user-facing display, "active" means "still usable." The docstring in codes.py documents this clearly. No action needed, just noting for future reference.

Backward compatibility: The location_name field is added to CodeResponse with str | None = None, so existing API consumers that do not expect this field will not break. The existing GET /locations/{id}/codes and POST /locations/{id}/codes now return location_name as enrichment only.

BLOCKERS

None.

New functionality has test coverage: 12 tests across 7 classes.
No unvalidated user input: enum-based status filter, keycloak_sub isolation on all queries.
No secrets or credentials in code.
No DRY violation in auth/security paths: auth is handled via the shared get_current_user dependency.

NITS

CodeResponse construction repeated 3 times (codes.py:72, locations.py:157, locations.py:196). All three manually map the same 9 fields. Consider a helper like CodeResponse.from_usage(usage, location_name=...) or a classmethod to reduce duplication. Not a blocker since this is not in an auth path, but if fields are added to CodeResponse later, all 3 sites must be updated in lockstep.
No pagination on GET /codes. For a personal tracker app, the dataset is likely small enough that this is fine. But if a user accumulates hundreds of codes over time, returning all in a single response could become slow. A limit/offset or cursor-based pagination would be a future improvement. Non-blocking for now.
Sort order test is weak (test_sorted_by_earned_at_desc). The test creates 3 codes in a tight loop without explicit earned_at values, relying on server_default=func.now() to produce distinct timestamps. In practice, all 3 rows may get the same func.now() value within a single transaction, making the sort order test pass vacuously (a sorted list equals itself reversed when all values are identical). Consider setting explicit earned_at values with timedelta offsets to make the test deterministic.
from_attributes = True unused on new code path. CodeResponse has model_config = ConfigDict(from_attributes=True), which allows direct ORM object mapping. The new endpoint manually constructs CodeResponse(...) from named Row tuple fields instead. This is fine and arguably more explicit, but worth noting that the from_attributes config is now vestigial for this particular endpoint's usage.

SOP COMPLIANCE

Branch named after issue: 14-add-get-codes-endpoint-enrich-coderespon (starts with #14)
PR body has: Summary, Changes, Test Plan, Related
Related references Closes #14
Related does not reference a plan slug (expected: plan-mcd-tracker)
Tests exist: 12 new tests, 130 total reported
No secrets committed
No unnecessary file changes (5 files, all directly related to the feature)
Commit messages are descriptive

PROCESS OBSERVATIONS

Deployment frequency: Clean single-feature PR. No scope creep. Ready for merge-and-deploy cycle.
Change failure risk: Low. Additive endpoint with no schema migration, no changes to existing query logic. Enrichment of existing responses is backward-compatible due to Optional field default.
Missing plan slug reference: The PR Related section has Closes #14 but does not reference plan-mcd-tracker. Minor process gap -- should include the plan slug for traceability.
Sort order test fragility (nit #3) could produce a false green. Low risk but worth fixing in a follow-up.

VERDICT: APPROVED

## PR #15 Review ### DOMAIN REVIEW **Tech stack**: Python 3.14 / FastAPI / SQLAlchemy / Pydantic / Alembic / PostgreSQL **Python/FastAPI/SQLAlchemy checklist**: - **PEP 484 type hints**: All function signatures and return types are properly annotated. `CodeStatus(str, Enum)` is correctly typed. - **PEP 257 docstrings**: New endpoint has a docstring explaining filter semantics. Module-level docstring present. - **SQLAlchemy patterns**: Single JOIN query (`CouponUsage JOIN Location`) avoids N+1. Explicit column selection in the query avoids loading unnecessary data. Session passed via FastAPI dependency injection -- correct pattern. - **FastAPI specifics**: `Query()` parameter with description for the status filter. `response_model=list[CodeResponse]` correctly declared. Enum-based query parameter gives automatic 422 on invalid values. - **OWASP / Input validation**: User isolation enforced via `CouponUsage.keycloak_sub == user.sub` in the JOIN filter. No raw SQL. Enum restricts status values to 3 known strings. No injection risk. - **Ruff compliance**: `# noqa: E712` comments on SQLAlchemy boolean comparisons (`== False`, `== True`) are correct -- SQLAlchemy requires this form for SQL generation. **Semantic observation -- "active" definition**: The `_count_active_codes()` function in `locations.py` (used for slot enforcement) defines "active" as `expires_at > now()` only, which **includes** redeemed codes. The new `GET /codes?status=active` defines "active" as `redeemed == False AND expires_at > now()`, which **excludes** redeemed codes. This is the correct design choice: for slot enforcement, a redeemed code still occupies a slot until expiry; for user-facing display, "active" means "still usable." The docstring in `codes.py` documents this clearly. No action needed, just noting for future reference. **Backward compatibility**: The `location_name` field is added to `CodeResponse` with `str | None = None`, so existing API consumers that do not expect this field will not break. The existing `GET /locations/{id}/codes` and `POST /locations/{id}/codes` now return `location_name` as enrichment only. ### BLOCKERS None. - New functionality has test coverage: 12 tests across 7 classes. - No unvalidated user input: enum-based status filter, keycloak_sub isolation on all queries. - No secrets or credentials in code. - No DRY violation in auth/security paths: auth is handled via the shared `get_current_user` dependency. ### NITS 1. **CodeResponse construction repeated 3 times** (`codes.py:72`, `locations.py:157`, `locations.py:196`). All three manually map the same 9 fields. Consider a helper like `CodeResponse.from_usage(usage, location_name=...)` or a classmethod to reduce duplication. Not a blocker since this is not in an auth path, but if fields are added to `CodeResponse` later, all 3 sites must be updated in lockstep. 2. **No pagination on `GET /codes`**. For a personal tracker app, the dataset is likely small enough that this is fine. But if a user accumulates hundreds of codes over time, returning all in a single response could become slow. A `limit`/`offset` or cursor-based pagination would be a future improvement. Non-blocking for now. 3. **Sort order test is weak** (`test_sorted_by_earned_at_desc`). The test creates 3 codes in a tight loop without explicit `earned_at` values, relying on `server_default=func.now()` to produce distinct timestamps. In practice, all 3 rows may get the same `func.now()` value within a single transaction, making the sort order test pass vacuously (a sorted list equals itself reversed when all values are identical). Consider setting explicit `earned_at` values with `timedelta` offsets to make the test deterministic. 4. **`from_attributes = True` unused on new code path**. `CodeResponse` has `model_config = ConfigDict(from_attributes=True)`, which allows direct ORM object mapping. The new endpoint manually constructs `CodeResponse(...)` from named Row tuple fields instead. This is fine and arguably more explicit, but worth noting that the `from_attributes` config is now vestigial for this particular endpoint's usage. ### SOP COMPLIANCE - [x] Branch named after issue: `14-add-get-codes-endpoint-enrich-coderespon` (starts with #14) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related references `Closes #14` - [ ] Related does not reference a plan slug (expected: `plan-mcd-tracker`) - [x] Tests exist: 12 new tests, 130 total reported - [x] No secrets committed - [x] No unnecessary file changes (5 files, all directly related to the feature) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean single-feature PR. No scope creep. Ready for merge-and-deploy cycle. - **Change failure risk**: Low. Additive endpoint with no schema migration, no changes to existing query logic. Enrichment of existing responses is backward-compatible due to `Optional` field default. - **Missing plan slug reference**: The PR Related section has `Closes #14` but does not reference `plan-mcd-tracker`. Minor process gap -- should include the plan slug for traceability. - **Sort order test fragility** (nit #3) could produce a false green. Low risk but worth fixing in a follow-up. ### VERDICT: APPROVED

2026-03-16 19:57:29 +00:00

Summary

Add a new GET /codes endpoint that lists all coupon codes for the authenticated user across all locations in a single JOIN query. Enrich CodeResponse with location_name field populated in both the new endpoint and existing location-scoped endpoints.

Changes

src/mcd_tracker_api/schemas.py: Added location_name: str | None = None to CodeResponse
src/mcd_tracker_api/routes/codes.py: New router with GET /codes endpoint. Single JOIN query (CouponUsage + Location), ?status=active|redeemed|expired filter via enum, sorted by earned_at desc
src/mcd_tracker_api/routes/locations.py: Updated log_code (POST) and list_codes (GET) to return location_name from the already-fetched Location object
src/mcd_tracker_api/main.py: Registered the new codes router
tests/test_codes.py: 12 new tests covering cross-location aggregation, location_name, sort order, user isolation, status filters, invalid status 422, and location_name in existing endpoints

Test Plan

Tests pass locally (130 total: 118 existing + 12 new)
ruff format and ruff check clean
Status filter semantics verified: active = not redeemed AND not expired, redeemed = redeemed flag true, expired = expires_at <= now
Existing GET /locations/{id}/codes unchanged in behavior, only enriched with location_name
No N+1 queries -- new endpoint uses a single JOIN

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #14

## Summary Add a new `GET /codes` endpoint that lists all coupon codes for the authenticated user across all locations in a single JOIN query. Enrich `CodeResponse` with `location_name` field populated in both the new endpoint and existing location-scoped endpoints. ## Changes - `src/mcd_tracker_api/schemas.py`: Added `location_name: str | None = None` to `CodeResponse` - `src/mcd_tracker_api/routes/codes.py`: New router with `GET /codes` endpoint. Single JOIN query (CouponUsage + Location), `?status=active|redeemed|expired` filter via enum, sorted by `earned_at` desc - `src/mcd_tracker_api/routes/locations.py`: Updated `log_code` (POST) and `list_codes` (GET) to return `location_name` from the already-fetched Location object - `src/mcd_tracker_api/main.py`: Registered the new codes router - `tests/test_codes.py`: 12 new tests covering cross-location aggregation, location_name, sort order, user isolation, status filters, invalid status 422, and location_name in existing endpoints ## Test Plan - [x] Tests pass locally (130 total: 118 existing + 12 new) - [x] `ruff format` and `ruff check` clean - [x] Status filter semantics verified: `active` = not redeemed AND not expired, `redeemed` = redeemed flag true, `expired` = expires_at <= now - [x] Existing `GET /locations/{id}/codes` unchanged in behavior, only enriched with `location_name` - [x] No N+1 queries -- new endpoint uses a single JOIN ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #14

forgejo_admin added 1 commit

feat: add GET /codes endpoint, enrich CodeResponse with location_name (#14 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

9cc081bdc0

Add a new GET /codes endpoint that lists all coupon codes for the
authenticated user across all locations via a single JOIN query.
Supports ?status=active|redeemed|expired filtering. Enrich CodeResponse
with location_name (str | None) populated in both the new endpoint and
existing POST/GET /locations/{id}/codes endpoints.

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