fix(yandex-valuation): parse removed_date, fix total_floors regex, normalize NBSP #526

Merged

lekss361 merged 1 commit from feat/tradein-yandex-valuation-parser-fixes into main

2026-05-24 14:12:17 +00:00

lekss361 commented

2026-05-24 14:00:09 +00:00

Owner

Summary

Add removed_date field to ValuationHistoryItem (extracts 2nd DD.MM.YYYY in chunk)
RE_FLOORS: singular этаж → plural этажей (was matching item-floor instead of dom-meta floors)
Normalize NBSP (0xA0) → space in parse() before regex (fixes *_price_per_m2 = NULL bug)
Add ValuationHouseMeta.validate_match() for cross-checking against expected house

Why

Bugs observed on prod: 160 rows in house_placement_history with all removed_date=NULL, total_floors=NULL, *_price_per_m2=NULL.

Root causes:

Regex grabbing first 'N этаж' from item rows, not 'M этажей' from dom-meta.
No NBSP normalization (Yandex SSR uses NBSP between digit groups and ₽).
parse_dmy() only returned first date — second date (removed_date) silently dropped.

Tests

25/25 pytest passing
ruff clean
9 new regression tests (removed_date extraction, in-sale flow, total_floors regression, NBSP price, validate_match scoring)

Followups (separate PRs)

PR #2: link house_placement_history.house_id FK (currently always NULL)
PR #3: bulk sweep all EKB yandex addresses

## Summary - Add `removed_date` field to `ValuationHistoryItem` (extracts 2nd DD.MM.YYYY in chunk) - `RE_FLOORS`: singular `этаж` → plural `этажей` (was matching item-floor instead of dom-meta floors) - Normalize NBSP (0xA0) → space in `parse()` before regex (fixes `*_price_per_m2` = NULL bug) - Add `ValuationHouseMeta.validate_match()` for cross-checking against expected house ## Why Bugs observed on prod: 160 rows in `house_placement_history` with all `removed_date=NULL`, `total_floors=NULL`, `*_price_per_m2=NULL`. Root causes: 1. Regex grabbing first 'N этаж' from item rows, not 'M этажей' from dom-meta. 2. No NBSP normalization (Yandex SSR uses NBSP between digit groups and ₽). 3. `parse_dmy()` only returned first date — second date (removed_date) silently dropped. ## Tests - 25/25 pytest passing - ruff clean - 9 new regression tests (removed_date extraction, in-sale flow, total_floors regression, NBSP price, validate_match scoring) ## Followups (separate PRs) - PR #2: link `house_placement_history.house_id` FK (currently always NULL) - PR #3: bulk sweep all EKB yandex addresses

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

fix(yandex-valuation): parse removed_date, fix total_floors regex, normalize NBSP 85f1f90410

- Add removed_date field to ValuationHistoryItem (extracts 2nd DD.MM.YYYY in chunk)
- RE_FLOORS: \d+\s+этаж → \d+\s+этажей (plural-only to avoid matching item floors)
- Normalize NBSP \xa0 → space in parse() before regex (fixes start_price_per_m2 / last_price_per_m2 = NULL bug)
- Add ValuationHouseMeta.validate_match() for cross-checking against expected house

Bugs observed on prod: 160 rows in house_placement_history with all
removed_date=NULL, total_floors=NULL, *_price_per_m2=NULL.

lekss361 commented

2026-05-24 14:11:55 +00:00

Author

Owner

Deep Code Review — PR #526

Summary

Status: APPROVE
Files: 2 (P1: yandex_valuation.py, P2: test file)
Lines: +156 / -31
Scope: parser fixes only (DB persistence is followup per author)

Three fixes — assessment

1. `removed_date` parsing — correct at parser level

re.finditer(r"\d{2}\.\d{2}\.\d{4}") collects ALL dates in chunk; first → publish_date, second → removed_date. Clean two-date semantics.
Test coverage covers two-date and "В продаже" (single-date) cases.
Edge case worth noting: chunk window (200 before + 100 after each date) can overlap adjacent items. If 2 items are <300 chars apart, the "second date" of chunk N may actually be the publish_date of item N+1, becoming a phantom removed_date. Dedup by (publish_date, area_m2, floor) mitigates duplicate rows but not phantom removed_date assignment. Real-page chunks are typically larger, so probably fine in practice; worth a regression probe after first prod sweep.

2. `RE_FLOORS` plural fix — correct, with one Russian-grammar edge case

Old (\d+)\s+этаж matched item-level "4 этаж" first → wrong. New (\d+)\s+этажей correctly targets dom-meta plural form. Tests confirm.
Russian paucal: "2 этажа" / "3 этажа" / "4 этажа" (low-rise 2–4 floor buildings) → new regex returns None. Single-floor "1 этаж" also won't match. Low impact for EKB highrise (almost everything is ≥5 floors → "этажей"), but consider a fallback for low-rise sweep: r"(\d+)\s+этаж(?:а|ей)?\b" combined with a check that it isn't inside an item context. Not blocking — explicit trade-off.

3. NBSP normalization — correctly placed

Applied at parse() entry on both raw HTML (line 216) and body_text (line 219). Belt-and-suspenders (handles literal \xa0 in source + entity-decoded   post-selectolax). All downstream regexes see clean spaces.
Pattern html.replace("\xa0", " ") is the canonical fix.
Note: other Yandex scrapers (yandex_realty, yandex_detail, yandex_newbuilding) may have the same NBSP issue with parse_rub (m.group(1).replace(" ", "") only strips regular space — \xa0 survives, breaks int()). Out of scope for this PR but worth a tracking note for the next cleanup sweep.

Important — Persistence gap (callout, not blocker)

estimator._save_yandex_history_items INSERT (estimator.py:457-476) binds only:
source, ext_item_id, rooms, area_m2, floor, start_price, start_price_date, last_price, last_price_date, exposure_days, raw_payload

removed_date and total_floors are NOT in the column list, and the rows dict (lines 446-455) does NOT include them. After this PR merges:

NBSP fix → start_price/last_price columns get correct values (NBSP-bearing prices were silently failing int() → NULL in DB). Net positive.
removed_date parsed → persisted only into raw_payload JSON, NOT to the dedicated house_placement_history.removed_date column. The 160-row NULL situation described in the PR body will persist for the DB column until persistence is updated.
Same for total_floors — parsed into ValuationHouseMeta.total_floors, lives in raw_payload JSON, but house_placement_history.total_floors column is not bound on INSERT.

Author's Followups section mentions PR #2 (house_id FK) and PR #3 (sweep). Suggest adding a followup: extend _save_yandex_history_items column list with removed_date, total_floors, and ensure last_price_date = removed_date OR publish_date so hph_removed_idx gets populated.

Low — `validate_match` has no consumer yet

New ValuationHouseMeta.validate_match() has 4 dedicated tests but no caller in repo. Looks intended for a future estimator integration (ambiguous-address geocode disambiguation). Fine as long as the followup arrives soon; otherwise dead code.

Cross-file impact

yandex_valuation.py is consumed by estimator.py (cache + persistence) and admin.py (admin trigger endpoint). Both consume result.history_items and result.house via Pydantic; the new optional removed_date field is backward-compatible (default None). No schema migration needed — column already exists from 017.
No frontend impact (admin trigger response shape unchanged — history_items_count still int).
Cache layer (external_valuations.raw_payload) stores full Pydantic dump via model_dump(mode="json") — removed_date round-trips correctly.
Stale cache entries (pre-PR) deserialized via model_validate(payload_dict) → removed_date defaults to None on missing key. Backward-compat OK.

Vault cross-check

Aligns with Schema_YandexRealty_Recon.md sec 12.5 + 13.2 (yandex_valuation as 4th source for house_placement_history).
Decision_TradeIn_DataQuality_8PR_Roadmap / Event_TradeIn_DataQuality_8PR_Done_May24 — PR #508 covered listing_date parsing; this PR (#526) continues the same Yandex-parser hardening line.

Tests

9 new regression tests cover all three fixes + 4 validate_match paths. Comprehensive.
25/25 passing per PR body.

Verdict reasoning

All three fixes are correct at the parser level, well-tested, and backward-compatible. The persistence gap is real but explicitly outside the parser scope per author's Followups. Approving — please bake the persistence-extension followup soon so the prod NULL situation actually clears.

Good scope discipline; tests are tight.

## Deep Code Review — PR #526 ### Summary - **Status**: APPROVE - **Files**: 2 (P1: yandex_valuation.py, P2: test file) - **Lines**: +156 / -31 - **Scope**: parser fixes only (DB persistence is followup per author) ### Three fixes — assessment #### 1. `removed_date` parsing — correct at parser level - `re.finditer(r"\d{2}\.\d{2}\.\d{4}")` collects ALL dates in chunk; first → `publish_date`, second → `removed_date`. Clean two-date semantics. - Test coverage covers two-date and "В продаже" (single-date) cases. - Edge case worth noting: chunk window (200 before + 100 after each date) can overlap adjacent items. If 2 items are <300 chars apart, the "second date" of chunk N may actually be the publish_date of item N+1, becoming a phantom removed_date. Dedup by `(publish_date, area_m2, floor)` mitigates duplicate rows but not phantom removed_date assignment. Real-page chunks are typically larger, so probably fine in practice; worth a regression probe after first prod sweep. #### 2. `RE_FLOORS` plural fix — correct, with one Russian-grammar edge case - Old `(\d+)\s+этаж` matched item-level "4 этаж" first → wrong. New `(\d+)\s+этажей` correctly targets dom-meta plural form. Tests confirm. - Russian paucal: "2 этажа" / "3 этажа" / "4 этажа" (low-rise 2–4 floor buildings) → new regex returns None. Single-floor "1 этаж" also won't match. Low impact for EKB highrise (almost everything is ≥5 floors → "этажей"), but consider a fallback for low-rise sweep: `r"(\d+)\s+этаж(?:а|ей)?\b"` combined with a check that it isn't inside an item context. Not blocking — explicit trade-off. #### 3. NBSP normalization — correctly placed - Applied at `parse()` entry on both raw HTML (line 216) and body_text (line 219). Belt-and-suspenders (handles literal `\xa0` in source + entity-decoded ` ` post-selectolax). All downstream regexes see clean spaces. - Pattern `html.replace("\xa0", " ")` is the canonical fix. - Note: other Yandex scrapers (yandex_realty, yandex_detail, yandex_newbuilding) may have the same NBSP issue with `parse_rub` (`m.group(1).replace(" ", "")` only strips regular space — `\xa0` survives, breaks `int()`). Out of scope for this PR but worth a tracking note for the next cleanup sweep. ### Important — Persistence gap (callout, not blocker) `estimator._save_yandex_history_items` INSERT (estimator.py:457-476) binds only: `source, ext_item_id, rooms, area_m2, floor, start_price, start_price_date, last_price, last_price_date, exposure_days, raw_payload` `removed_date` and `total_floors` are NOT in the column list, and the `rows` dict (lines 446-455) does NOT include them. After this PR merges: - NBSP fix → `start_price`/`last_price` columns get correct values (NBSP-bearing prices were silently failing `int()` → NULL in DB). Net positive. - `removed_date` parsed → persisted only into `raw_payload` JSON, NOT to the dedicated `house_placement_history.removed_date` column. The 160-row NULL situation described in the PR body will **persist** for the DB column until persistence is updated. - Same for `total_floors` — parsed into `ValuationHouseMeta.total_floors`, lives in `raw_payload` JSON, but `house_placement_history.total_floors` column is not bound on INSERT. Author's Followups section mentions PR #2 (`house_id` FK) and PR #3 (sweep). Suggest adding a followup: extend `_save_yandex_history_items` column list with `removed_date`, `total_floors`, and ensure `last_price_date = removed_date OR publish_date` so `hph_removed_idx` gets populated. ### Low — `validate_match` has no consumer yet - New `ValuationHouseMeta.validate_match()` has 4 dedicated tests but no caller in repo. Looks intended for a future estimator integration (ambiguous-address geocode disambiguation). Fine as long as the followup arrives soon; otherwise dead code. ### Cross-file impact - `yandex_valuation.py` is consumed by `estimator.py` (cache + persistence) and `admin.py` (admin trigger endpoint). Both consume `result.history_items` and `result.house` via Pydantic; the new optional `removed_date` field is backward-compatible (default None). No schema migration needed — column already exists from 017. - No frontend impact (admin trigger response shape unchanged — `history_items_count` still int). - Cache layer (`external_valuations.raw_payload`) stores full Pydantic dump via `model_dump(mode="json")` — `removed_date` round-trips correctly. - Stale cache entries (pre-PR) deserialized via `model_validate(payload_dict)` → `removed_date` defaults to None on missing key. Backward-compat OK. ### Vault cross-check - Aligns with `Schema_YandexRealty_Recon.md` sec 12.5 + 13.2 (`yandex_valuation` as 4th source for `house_placement_history`). - `Decision_TradeIn_DataQuality_8PR_Roadmap` / `Event_TradeIn_DataQuality_8PR_Done_May24` — PR #508 covered `listing_date` parsing; this PR (#526) continues the same Yandex-parser hardening line. ### Tests - 9 new regression tests cover all three fixes + 4 `validate_match` paths. Comprehensive. - 25/25 passing per PR body. ### Verdict reasoning All three fixes are correct at the parser level, well-tested, and backward-compatible. The persistence gap is real but explicitly outside the parser scope per author's Followups. Approving — please bake the persistence-extension followup soon so the prod NULL situation actually clears. Good scope discipline; tests are tight.