lekss361/gendesign

Fork 0

feat(tradein-api): surface backfilled houses + placement history #528

Merged

lekss361 merged 1 commit from feat/tradein-surface-houses-history-api into main

2026-05-24 14:22:51 +00:00

lekss361 commented

2026-05-24 14:16:56 +00:00

Owner

Context

PR #527 bootstrap'нул 6,507 houses (562 Avito + 5,945 derived) и слинковал 18,197 listings. Существующий /estimate/{id}/houses фильтровал source='avito' (мало совпадений), house_placement_history вообще не surfaced.

Changes

`GET /estimate/{id}/houses` (reworked)

Два пути + union-dedup:

Direct match by normalized address (tradein_normalize_short_addr SQL func из PR #527)
Geo-nearby в 500м, ANY source (фильтр 'avito' удалён)

Requires: estimate's address field (раньше только lat/lon).

`GET /estimate/{id}/placement-history` (new)

Ищет house_ids по адресу target → возвращает до 50 lots из house_placement_history отсортированных по last_price_date DESC. Геофоллбэк 100м если addr пустой.

`HouseInfoForEstimate.source: str | None`

Добавлено поле — на UI видно avito / derived / cian_newbuilding etc.

Roadmap

Follow-up to PR #527 (backfill) и [[Decision_TradeIn_DataQuality_8PR_Roadmap]]. UI part — отдельный PR (frontend PlacementHistoryCard).

Verify

curl -u admin:… https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<uuid>/houses | jq '.[] | {source, address, year_built, total_floors, distance_m}'
curl -u admin:… https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<uuid>/placement-history | jq 'length'

Dependencies

Merge после #527 (нужен tradein_normalize_short_addr функция в БД).

## Context PR #527 bootstrap'нул 6,507 houses (562 Avito + 5,945 derived) и слинковал 18,197 listings. Существующий `/estimate/{id}/houses` фильтровал `source='avito'` (мало совпадений), `house_placement_history` вообще не surfaced. ## Changes ### `GET /estimate/{id}/houses` (reworked) Два пути + union-dedup: 1. **Direct match** by normalized address (`tradein_normalize_short_addr` SQL func из PR #527) 2. **Geo-nearby** в 500м, ANY source (фильтр `'avito'` удалён) Requires: estimate's `address` field (раньше только `lat/lon`). ### `GET /estimate/{id}/placement-history` (new) Ищет `house_ids` по адресу target → возвращает до 50 lots из `house_placement_history` отсортированных по `last_price_date DESC`. Геофоллбэк 100м если addr пустой. ### `HouseInfoForEstimate.source: str | None` Добавлено поле — на UI видно `avito` / `derived` / `cian_newbuilding` etc. ## Roadmap Follow-up to PR #527 (backfill) и `[[Decision_TradeIn_DataQuality_8PR_Roadmap]]`. UI part — отдельный PR (frontend `PlacementHistoryCard`). ## Verify ```bash curl -u admin:… https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<uuid>/houses | jq '.[] | {source, address, year_built, total_floors, distance_m}' curl -u admin:… https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<uuid>/placement-history | jq 'length' ``` ## Dependencies Merge после #527 (нужен `tradein_normalize_short_addr` функция в БД).

lekss361 added 1 commit 2026-05-24 14:16:56 +00:00

feat(tradein-api): surface backfilled houses + placement history 9c4c13780e

After PR #527 bootstrap'нул 6,507 houses and linked 18,197 listings.
- /estimate/{id}/houses no longer filters source='avito' — returns direct
  match по нормализованному address + nearby houses (any source).
- New /estimate/{id}/placement-history returns historical lots from
  house_placement_history for the target house(s).

lekss361 commented

2026-05-24 14:22:35 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE
Files: 2 (P0: 1 / P1: 1) · +145 / -31
PR: #528 · base: main · head: 9c4c137
Dependencies: PR #527 merged at 2026-05-24T14:17:32Z — tradein_normalize_short_addr function + houses bootstrap confirmed deployed.

Correctness — verified

CAST(:id AS uuid) used everywhere — psycopg v3 contract respected.
All house_placement_history columns in SELECT exist (017 + 045 migrations).
houses.short_address is populated for derived rows (063 Step 3) but NOT for non-derived (avito/cian) rows. The endpoint's WHERE clause has OR tradein_normalize_short_addr(h.address) = ... as fallback — covers both cases correctly.
Dedup merge direct + nearby by house_id is correct.
house_ids = ANY(:house_ids) with Python list[int] — psycopg v3 native array binding.
f-string interpolation of _HOUSE_SELECT_COLS is static (no user input) — no SQL injection.

Findings (non-blocking)

Medium — `placement-history` lacks `response_model`

tradein-mvp/backend/app/api/v1/trade_in.py:434 — def get_estimate_placement_history(...) -> list[dict] без Pydantic schema. Inconsistent with /houses (response_model=list[HouseInfoForEstimate]) и /imv-benchmark (IMVBenchmarkResponse). Consequences: нет OpenAPI schema, нет validation, UI consumes Any. Follow-up suggestion:

class PlacementHistoryEntry(BaseModel):
    id: int
    source: str
    house_id: int | None = None
    ext_item_id: str
    title: str | None = None
    rooms: int | None = None
    area_m2: float | None = None
    floor: int | None = None
    total_floors: int | None = None
    start_price: int | None = None
    start_price_date: date | None = None
    last_price: int | None = None
    last_price_date: date | None = None
    removed_date: date | None = None
    exposure_days: int | None = None
    notes: str | None = None

Then @router.get(..., response_model=list[PlacementHistoryEntry]).

Medium — truthy lat/lon check

trade_in.py:411, 484 — if target.lat and target.lon: ловит lat=0.0 как falsy. Для EKB (lat≈56.8) невозможно, но проектный стандарт is not None. Defends against future region expansion.

Low — no `houses(short_address)` index

Migration 063 explicitly notes this is acceptable for one-shot. На каждом запросе делается seq scan по 6.5k rows (~ms сейчас). Follow-up для full sweep target (≥10k houses):

CREATE INDEX CONCURRENTLY IF NOT EXISTS houses_short_addr_idx
  ON houses(short_address) WHERE short_address IS NOT NULL;

Low — hardcoded LIMIT 50, no pagination

OK for current 160-row table. Add limit: int = Query(50, le=200) if growth expected.

Low — `from typing import Any`

Used только в list[Any] = [] для row mappings — мог бы быть list без аннотации. Стилистическое.

Vault / memory cross-check

CAST IS NOT NULL trap (PR #509→#518): N/A, никаких :param IS NOT NULL предикатов.
Dependency contract verification: confirmed PR #527 merged, tradein_normalize_short_addr deployed.
Backfill migration checklist: was PR #527's scope, not this PR.

Cross-file impact

HouseInfoForEstimate.source: str | None — additive optional поле, backward-compatible. Frontend consumers старых версий продолжат работать (просто не покажут source label).
Old endpoint contract: убран source='avito' filter — это intentional roadmap step (см. body PR). Frontend сейчас не использует source-specific логику в /houses.

Risk & blast radius

Risk: low. Single router file, two endpoints (one reworked, one new).
Reversibility: high (no DB DDL).
Merge window: safe.

## Deep Code Review — verdict ### Summary - Status: APPROVE - Files: 2 (P0: 1 / P1: 1) · +145 / -31 - PR: #528 · base: main · head: 9c4c137 - Dependencies: PR #527 merged at 2026-05-24T14:17:32Z — `tradein_normalize_short_addr` function + houses bootstrap confirmed deployed. ### Correctness — verified - `CAST(:id AS uuid)` used everywhere — psycopg v3 contract respected. - All `house_placement_history` columns in SELECT exist (017 + 045 migrations). - `houses.short_address` is populated for `derived` rows (063 Step 3) but NOT for non-derived (avito/cian) rows. The endpoint's WHERE clause has `OR tradein_normalize_short_addr(h.address) = ...` as fallback — covers both cases correctly. - Dedup merge `direct + nearby` by `house_id` is correct. - `house_ids = ANY(:house_ids)` with Python `list[int]` — psycopg v3 native array binding. - f-string interpolation of `_HOUSE_SELECT_COLS` is static (no user input) — no SQL injection. ### Findings (non-blocking) #### Medium — `placement-history` lacks `response_model` `tradein-mvp/backend/app/api/v1/trade_in.py:434` — `def get_estimate_placement_history(...) -> list[dict]` без Pydantic schema. Inconsistent with `/houses` (`response_model=list[HouseInfoForEstimate]`) и `/imv-benchmark` (`IMVBenchmarkResponse`). Consequences: нет OpenAPI schema, нет validation, UI consumes `Any`. Follow-up suggestion: ```python class PlacementHistoryEntry(BaseModel): id: int source: str house_id: int | None = None ext_item_id: str title: str | None = None rooms: int | None = None area_m2: float | None = None floor: int | None = None total_floors: int | None = None start_price: int | None = None start_price_date: date | None = None last_price: int | None = None last_price_date: date | None = None removed_date: date | None = None exposure_days: int | None = None notes: str | None = None ``` Then `@router.get(..., response_model=list[PlacementHistoryEntry])`. #### Medium — truthy lat/lon check `trade_in.py:411, 484` — `if target.lat and target.lon:` ловит `lat=0.0` как falsy. Для EKB (lat≈56.8) невозможно, но проектный стандарт `is not None`. Defends against future region expansion. #### Low — no `houses(short_address)` index Migration 063 explicitly notes this is acceptable for one-shot. На каждом запросе делается seq scan по 6.5k rows (~ms сейчас). Follow-up для full sweep target (≥10k houses): ```sql CREATE INDEX CONCURRENTLY IF NOT EXISTS houses_short_addr_idx ON houses(short_address) WHERE short_address IS NOT NULL; ``` #### Low — hardcoded LIMIT 50, no pagination OK for current 160-row table. Add `limit: int = Query(50, le=200)` if growth expected. #### Low — `from typing import Any` Used только в `list[Any] = []` для row mappings — мог бы быть `list` без аннотации. Стилистическое. ### Vault / memory cross-check - CAST IS NOT NULL trap (PR #509→#518): N/A, никаких `:param IS NOT NULL` предикатов. - Dependency contract verification: confirmed PR #527 merged, `tradein_normalize_short_addr` deployed. - Backfill migration checklist: was PR #527's scope, not this PR. ### Cross-file impact - `HouseInfoForEstimate.source: str | None` — additive optional поле, backward-compatible. Frontend consumers старых версий продолжат работать (просто не покажут source label). - Old endpoint contract: убран `source='avito'` filter — это intentional roadmap step (см. body PR). Frontend сейчас не использует source-specific логику в `/houses`. ### Risk & blast radius - Risk: low. Single router file, two endpoints (one reworked, one new). - Reversibility: high (no DB DDL). - Merge window: safe.