feat(tradein-estimator): exclude rosreestr deals from output #504

Merged

lekss361 merged 1 commit from feat/tradein-drop-rosreestr-deals into main

2026-05-24 10:52:56 +00:00

lekss361 commented

2026-05-24 10:29:38 +00:00

Owner

Problem

Production estimate a0a0b820-e8a8-4eee-aa73-0ab3b98ac233:

analogs (cian/yandex/avito): 105–126 К/м² (tight ±6%)
deals from rosreestr: 99–200 К/м² (2× spread, median ~163К)

rosreestr contains ДДУ first-market contracts that don't represent secondary market. They skew actual_deals median by +40%.

Fix

estimate_quality() passes deals: list = [] instead of calling _fetch_deals()
_fetch_deals() helper kept in place (PR 8 cleanup is a separate concern; may re-enable per-segment later)
Frontend handling deferred to PR 7

Roadmap

PR 2 of 8 — see [[Decision_TradeIn_DataQuality_8PR_Roadmap]].

Verify

curl -u admin:... https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<fresh-uuid> | jq .actual_deals
# expected: []

## Problem Production estimate `a0a0b820-e8a8-4eee-aa73-0ab3b98ac233`: - analogs (cian/yandex/avito): 105–126 К/м² (tight ±6%) - deals from rosreestr: 99–200 К/м² (2× spread, median ~163К) rosreestr contains ДДУ first-market contracts that don't represent secondary market. They skew `actual_deals` median by +40%. ## Fix - `estimate_quality()` passes `deals: list = []` instead of calling `_fetch_deals()` - `_fetch_deals()` helper kept in place (PR 8 cleanup is a separate concern; may re-enable per-segment later) - Frontend handling deferred to PR 7 ## Roadmap PR 2 of 8 — see `[[Decision_TradeIn_DataQuality_8PR_Roadmap]]`. ## Verify ```bash curl -u admin:... https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<fresh-uuid> | jq .actual_deals # expected: [] ```

lekss361 added 1 commit 2026-05-24 10:29:39 +00:00

feat(tradein-estimator): exclude rosreestr deals from actual_deals output 4b32b263e1

rosreestr_deals contains застройщик DDU contracts (177-200 К/м²) that skew
median for secondary-market valuation (~114 К/м²). Pass empty deals list to
AggregatedEstimate; keep _fetch_deals() helper intact for future
segmentation work.

Source: estimate a0a0b820-e8a8-4eee-aa73-0ab3b98ac233 (deals median +40% vs analogs).

lekss361 commented

2026-05-24 10:39:01 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE (ready for human merge — blocked scope services/)
Files: 1 (P1: estimator.py)
Lines: +10 / -6 · PR: #504
Decision authority: Decision_TradeIn_DataQuality_8PR_Roadmap (PR 2 of 8) — exact match

Critical / High / Medium

None.

Cross-file impact (verified)

With deals: list[dict[str, Any]] = [] (line 595), three downstream consumers all degrade gracefully:

estimator.py:600 — deals_lots = [_deal_to_analog(d) for d in deals[:10]] → [].
estimator.py:658 — deals_json insert payload → [] JSON. actual_deals JSONB column receives empty array. Schema unchanged → DB backward compat OK for legacy estimates.
estimator.py:726 — _estimate_days_on_market(listings_clean, deals). With deals=[], (*listings, *deals) collapses to listings only. Existing len(values) < 3 → None fallback already handles thin data → no break, but ETA precision slightly degrades. Acceptable per Decision (deals были искажающим сигналом, не точным).
_fetch_deals helper at line 928 — preserved per PR body and roadmap (PR 8 cleanup separate). No dead-code blocker.
AggregatedEstimate.actual_deals (response) → []. Frontend handling deferred to PR 7 of roadmap — explicit scope split.
Any import present at line 26 — type annotation legal.

Performance

Small WIN: removed PostGIS query against partitioned rosreestr_deals table per estimate call. No regression.

Test coverage

No unit test update in diff. Acceptable:

Change is behavioural-flip (always-empty), not algorithmic.
PR body provides explicit curl acceptance criterion: jq .actual_deals == [].
Existing test_estimator_cian_integration.py does not test actual_deals shape (only cache_key logic).
Post-deploy qa-tester curl verify on fresh UUID is sufficient gate.

Vault cross-check

Decision_TradeIn_DataQuality_8PR_Roadmap.md PR 2 specification matches diff verbatim:

✅ estimator.py: skip _fetch_deals(), pass deals=[]
✅ schema/API unchanged (legacy estimates preserved)
✅ helper not removed (deferred to PR 8)
✅ frontend deferred to PR 7

NOTE/comment in docstring + inline at line 588-592 reference the Decision — good audit trail.

Positive

Tiny, surgical, reversible change
Inline comment with date + Decision name (auditability)
Explicit type annotation list[dict[str, Any]] = [] keeps downstream type-narrowing intact
Helper preservation respects scope split

Recommended next steps (post-merge, not blockers)

Post-deploy qa-tester: curl -u admin:… https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<fresh-uuid> | jq '.actual_deals, .est_days_on_market'
Watch GlitchTip for any KeyError near _estimate_days_on_market (degenerate listings-only case)
Proceed with PR 1 (Avito address CSS clean) and PR 7 (frontend section hide) per roadmap

Complexity / blast radius

Risk: LOW (behaviour-flip, no schema change, no contract break)
Reversibility: trivial (revert single line)
Merge window: any

✅ ready for human merge

## Deep Code Review — verdict ### Summary - Status: APPROVE (ready for human merge — blocked scope `services/`) - Files: 1 (P1: estimator.py) - Lines: +10 / -6 · PR: #504 - Decision authority: `Decision_TradeIn_DataQuality_8PR_Roadmap` (PR 2 of 8) — exact match ### Critical / High / Medium None. ### Cross-file impact (verified) With `deals: list[dict[str, Any]] = []` (line 595), three downstream consumers all degrade gracefully: 1. `estimator.py:600` — `deals_lots = [_deal_to_analog(d) for d in deals[:10]]` → `[]`. 2. `estimator.py:658` — `deals_json` insert payload → `[]` JSON. `actual_deals` JSONB column receives empty array. Schema unchanged → DB backward compat OK for legacy estimates. 3. `estimator.py:726` — `_estimate_days_on_market(listings_clean, deals)`. With `deals=[]`, `(*listings, *deals)` collapses to listings only. Existing `len(values) < 3 → None` fallback already handles thin data → no break, but ETA precision slightly degrades. Acceptable per Decision (deals были искажающим сигналом, не точным). 4. `_fetch_deals` helper at line 928 — preserved per PR body and roadmap (PR 8 cleanup separate). No dead-code blocker. 5. `AggregatedEstimate.actual_deals` (response) → `[]`. Frontend handling deferred to PR 7 of roadmap — explicit scope split. 6. `Any` import present at line 26 — type annotation legal. ### Performance Small WIN: removed PostGIS query against partitioned `rosreestr_deals` table per estimate call. No regression. ### Test coverage No unit test update in diff. Acceptable: - Change is behavioural-flip (always-empty), not algorithmic. - PR body provides explicit curl acceptance criterion: `jq .actual_deals == []`. - Existing `test_estimator_cian_integration.py` does not test `actual_deals` shape (only cache_key logic). - Post-deploy `qa-tester` curl verify on fresh UUID is sufficient gate. ### Vault cross-check `Decision_TradeIn_DataQuality_8PR_Roadmap.md` PR 2 specification matches diff verbatim: - ✅ estimator.py: skip `_fetch_deals()`, pass `deals=[]` - ✅ schema/API unchanged (legacy estimates preserved) - ✅ helper not removed (deferred to PR 8) - ✅ frontend deferred to PR 7 NOTE/comment in docstring + inline at line 588-592 reference the Decision — good audit trail. ### Positive - Tiny, surgical, reversible change - Inline comment with date + Decision name (auditability) - Explicit type annotation `list[dict[str, Any]] = []` keeps downstream type-narrowing intact - Helper preservation respects scope split ### Recommended next steps (post-merge, not blockers) 1. Post-deploy `qa-tester`: `curl -u admin:… https://gendsgn.ru/trade-in/api/v1/trade-in/estimate/<fresh-uuid> | jq '.actual_deals, .est_days_on_market'` 2. Watch GlitchTip for any KeyError near `_estimate_days_on_market` (degenerate listings-only case) 3. Proceed with PR 1 (Avito address CSS clean) and PR 7 (frontend section hide) per roadmap ### Complexity / blast radius - Risk: LOW (behaviour-flip, no schema change, no contract break) - Reversibility: trivial (revert single line) - Merge window: any ✅ ready for human merge