fix(tradein-api): PlacementHistoryEntry schema + None checks (PR #528 follow-up) #532

Merged

lekss361 merged 1 commit from fix/tradein-placement-history-schema into main

2026-05-24 14:33:54 +00:00

lekss361 commented

2026-05-24 14:26:28 +00:00

Owner

Context

Follow-up to PR #528 review-bot APPROVE with 2 non-blocking Medium findings.

Changes

1. `PlacementHistoryEntry` Pydantic schema

schemas/trade_in.py — добавлена 17-field BaseModel matching house_placement_history columns (id, source, house_id, ext_item_id, title, rooms, area_m2, floor, total_floors, start_price/date, last_price/date, removed_date, exposure_days, notes). Размещена между IMVBenchmarkResponse и ScheduleConfig.

api/v1/trade_in.py:

@router.get("/estimate/{estimate_id}/placement-history", response_model=list[PlacementHistoryEntry]) — было list[dict] без типа
Return: [PlacementHistoryEntry(**dict(r)) for r in history] (Pydantic validation + OpenAPI doc)

Эффект: consistent с /houses (list[HouseInfoForEstimate]) и /imv-benchmark (IMVBenchmarkResponse). UI получает typed response.

2. Truthy lat/lon → `is not None`

api/v1/trade_in.py:421, 493 — было if target.lat and target.lon:, теперь is not None. Defensive против lat=0.0 (EKB lat≈56.8 безопасен, но при region expansion в Калининград/Сахалин 0 теоретически возможен).

Roadmap

Follow-up к PR #528. Не блокер был, no production impact. Полная цепочка backfill+UI: #527 → #528 → #529 → this PR.

Verify

curl -u admin:… https://gendsgn.ru/trade-in/api/v1/openapi.json | jq '.components.schemas.PlacementHistoryEntry'
# expected: full schema with all fields

## Context Follow-up to **PR #528** review-bot APPROVE with 2 non-blocking Medium findings. ## Changes ### 1. `PlacementHistoryEntry` Pydantic schema `schemas/trade_in.py` — добавлена 17-field BaseModel matching `house_placement_history` columns (id, source, house_id, ext_item_id, title, rooms, area_m2, floor, total_floors, start_price/date, last_price/date, removed_date, exposure_days, notes). Размещена между `IMVBenchmarkResponse` и `ScheduleConfig`. `api/v1/trade_in.py`: - `@router.get("/estimate/{estimate_id}/placement-history", response_model=list[PlacementHistoryEntry])` — было `list[dict]` без типа - Return: `[PlacementHistoryEntry(**dict(r)) for r in history]` (Pydantic validation + OpenAPI doc) **Эффект:** consistent с `/houses` (`list[HouseInfoForEstimate]`) и `/imv-benchmark` (`IMVBenchmarkResponse`). UI получает typed response. ### 2. Truthy lat/lon → `is not None` `api/v1/trade_in.py:421, 493` — было `if target.lat and target.lon:`, теперь `is not None`. Defensive против `lat=0.0` (EKB lat≈56.8 безопасен, но при region expansion в Калининград/Сахалин 0 теоретически возможен). ## Roadmap Follow-up к PR #528. Не блокер был, no production impact. Полная цепочка backfill+UI: #527 → #528 → #529 → this PR. ## Verify ```bash curl -u admin:… https://gendsgn.ru/trade-in/api/v1/openapi.json | jq '.components.schemas.PlacementHistoryEntry' # expected: full schema with all fields ```

lekss361 added 1 commit 2026-05-24 14:26:29 +00:00

fix(tradein-api): add PlacementHistoryEntry schema + None checks 72334a7558

Follow-up to PR #528 review-bot Medium findings:
- /placement-history endpoint typed via new PlacementHistoryEntry schema
  (OpenAPI + Pydantic validation, consistent with other endpoints)
- Replace truthy 'if target.lat and target.lon' with 'is not None'
  (defensive against lat=0.0 in future region expansion)

lekss361 commented

2026-05-24 14:33:47 +00:00

Author

Owner

Deep Code Review — verdict: APPROVE

Summary

Status: APPROVE
Files: 2 (P1: api/v1/trade_in.py, schemas/trade_in.py)
Lines: +27 / -5
Scope: follow-up на #528 medium notes (response_model + falsy lat/lon)

Verified

PlacementHistoryEntry schema cross-check — все 16 полей matches house_placement_history (017_house_placement_history.sql + 045_extend.sql):
- id bigserial → int ✓
- source text NOT NULL → str (required) ✓
- house_id bigint REFERENCES houses (nullable per DDL) → int | None ✓
- ext_item_id text NOT NULL → str (required) ✓
- area_m2 numeric(8,2) → float | None ✓ (Pydantic v2 lax mode coerces Decimal→float)
- start_price/last_price bigint → int | None ✓
- start_price_date/last_price_date/removed_date date → date | None ✓ (psycopg v3 returns native datetime.date)
- exposure_days int, rooms, floor, total_floors, title, notes — nullable correctly ✓
response_model=list[PlacementHistoryEntry] правильно подключён. JSON shape backward-compatible — keys те же, dates сериализуются в ISO, Decimal→float. Frontend PlacementHistoryCard (#529) gracefully handles все nullable поля (fmtPrice(null) → "—", lotLabel defaults).
is not None fix — корректное закрытие edge case lat=0.0/lon=0.0:
- trade_in_estimates.lat/lon = double precision (per 001_trade_in_estimates.sql)
- 0.0 в if x and y: падал в false-branch (skip geo fallback) — bug
- is not None теперь корректно разрешает 0.0 (хоть и не realistic для ЕКБ, но семантика правильная)
- Применён в обоих местах (line 421 + 493) консистентно ✓
Query безопасность — WHERE house_id = ANY(:house_ids) фильтрует NULL house_id, поэтому frontend никогда не получит null house_id (de-facto backward-compat с PlacementHistoryItem.house_id: number в #529).
No CAST/IS NULL trap — is not None тут в Python-коде, не в SQL bound params; psycopg v3 AmbiguousParameter не релевантен.

Notes (non-blocking, follow-up если захотите)

Frontend type PlacementHistoryItem.house_id: number (#529) можно ослабить до number | null для type-parity с backend int | None. Не влияет на runtime (SQL фильтрует NULL).
Regression test для /placement-history endpoint отсутствует — consistent с codebase style (tests/ покрывают только parsers), OK как nit.

Risk / blast radius

Low. Изоляция: 2 файла, нет cross-domain effects.
Reversibility: trivial revert (1 commit).

Verdict

Merge ready.

## Deep Code Review — verdict: APPROVE ### Summary - Status: APPROVE - Files: 2 (P1: api/v1/trade_in.py, schemas/trade_in.py) - Lines: +27 / -5 - Scope: follow-up на #528 medium notes (response_model + falsy lat/lon) ### Verified 1. **`PlacementHistoryEntry` schema cross-check** — все 16 полей matches `house_placement_history` (017_house_placement_history.sql + 045_extend.sql): - `id bigserial` → `int` ✓ - `source text NOT NULL` → `str` (required) ✓ - `house_id bigint REFERENCES houses` (nullable per DDL) → `int | None` ✓ - `ext_item_id text NOT NULL` → `str` (required) ✓ - `area_m2 numeric(8,2)` → `float | None` ✓ (Pydantic v2 lax mode coerces Decimal→float) - `start_price/last_price bigint` → `int | None` ✓ - `start_price_date/last_price_date/removed_date date` → `date | None` ✓ (psycopg v3 returns native `datetime.date`) - `exposure_days int`, `rooms`, `floor`, `total_floors`, `title`, `notes` — nullable correctly ✓ 2. **`response_model=list[PlacementHistoryEntry]`** правильно подключён. JSON shape **backward-compatible** — keys те же, dates сериализуются в ISO, Decimal→float. Frontend `PlacementHistoryCard` (#529) gracefully handles все nullable поля (`fmtPrice(null) → "—"`, `lotLabel` defaults). 3. **`is not None` fix** — корректное закрытие edge case lat=0.0/lon=0.0: - `trade_in_estimates.lat/lon` = `double precision` (per 001_trade_in_estimates.sql) - 0.0 в `if x and y:` падал в false-branch (skip geo fallback) — bug - `is not None` теперь корректно разрешает 0.0 (хоть и не realistic для ЕКБ, но семантика правильная) - Применён в обоих местах (line 421 + 493) консистентно ✓ 4. **Query безопасность** — `WHERE house_id = ANY(:house_ids)` фильтрует NULL house_id, поэтому frontend никогда не получит null house_id (de-facto backward-compat с `PlacementHistoryItem.house_id: number` в #529). 5. **No CAST/IS NULL trap** — `is not None` тут в Python-коде, не в SQL bound params; psycopg v3 AmbiguousParameter не релевантен. ### Notes (non-blocking, follow-up если захотите) - Frontend type `PlacementHistoryItem.house_id: number` (#529) можно ослабить до `number | null` для type-parity с backend `int | None`. Не влияет на runtime (SQL фильтрует NULL). - Regression test для `/placement-history` endpoint отсутствует — consistent с codebase style (tests/ покрывают только parsers), OK как nit. ### Risk / blast radius - Low. Изоляция: 2 файла, нет cross-domain effects. - Reversibility: trivial revert (1 commit). ### Verdict Merge ready.