diff --git a/core/integrations/market/rentometer.py b/core/integrations/market/rentometer.py index f7d7998..a628078 100644 --- a/core/integrations/market/rentometer.py +++ b/core/integrations/market/rentometer.py @@ -9,12 +9,14 @@ from __future__ import annotations +import hashlib import logging -from decimal import Decimal +from decimal import Decimal, InvalidOperation from typing import Any, cast import requests from django.conf import settings +from django.core.cache import cache logger = logging.getLogger(__name__) @@ -39,15 +41,28 @@ def _headers(self) -> dict[str, str]: def _get(self, path: str, params: dict[str, str] | None = None) -> dict[str, Any]: """Make a GET request to the Rentometer API.""" url = f"{RENTO_METER_API_BASE}/{path.lstrip('/')}" - resp = requests.get( - url, headers=self._headers(), params=params, timeout=REQUEST_TIMEOUT - ) - if resp.status_code == 401: - raise RentometerError("Rentometer API key is missing or invalid") - if resp.status_code == 404: - raise RentometerError(f"No data found: {path}") - resp.raise_for_status() - data = resp.json() + try: + resp = requests.get( + url, headers=self._headers(), params=params, timeout=REQUEST_TIMEOUT + ) + if resp.status_code == 401: + raise RentometerError("Rentometer API key is missing or invalid") + if resp.status_code == 404: + raise RentometerError(f"No data found: {path}") + resp.raise_for_status() + data = resp.json() + except RentometerError: + raise + except requests.RequestException as exc: + # Timeouts, connection errors, and other non-2xx status codes + # (e.g. 429 rate-limit, 5xx) all funnel through RentometerError + # so every caller's single `except RentometerError` catches and + # logs them, instead of a raw requests exception escaping + # uncaught. + raise RentometerError(f"Rentometer request failed: {exc}") from exc + except ValueError as exc: + # resp.json() raises a ValueError subclass on malformed JSON. + raise RentometerError(f"Rentometer returned invalid JSON: {exc}") from exc return cast(dict[str, Any], data) def get_rent_by_address( @@ -91,20 +106,30 @@ def _parse_rent(value: Any) -> Decimal | None: if value is None or value == "": return None try: - val = float(value) - if val <= 0: - return None - return Decimal(str(round(val, 2))) - except ValueError, TypeError: + dec = Decimal(str(value)).quantize(Decimal("0.01")) + except InvalidOperation, ValueError, TypeError: + return None + if dec <= 0: return None + return dec -# ── In-memory cache (simple TTL cache for 7-day window) ────────────────── +# ── Cache (Django's cache framework — same pattern as walkscore.py) ────── -_rent_cache: dict[str, tuple[float, dict[str, Any] | None]] = {} _CACHE_TTL_SECONDS = CACHE_TTL_DAYS * 86400 +def _cache_key( + address: str | None, + city: str | None, + state: str | None, + zip_code: str | None, + bedrooms: int, +) -> str: + raw = f"{address or ''}|{city or ''}|{state or ''}|{zip_code or ''}|{bedrooms}" + return f"rentometer_{hashlib.sha256(raw.encode()).hexdigest()}" + + def get_rent_estimate( zip_code: str | None = None, address: str | None = None, @@ -121,40 +146,33 @@ def get_rent_estimate( address: Street address (improves accuracy). city: City name. state: 2-letter state code. - bedrooms: Number of bedrooms (used for future bed-specific lookups). + bedrooms: Number of bedrooms. Used today only to differentiate cache + entries (not yet passed to the Rentometer API call itself). Returns: Monthly median rent as Decimal, or None if unavailable. """ - import time - client = RentometerClient() if not client.api_key: logger.debug("RENTO_METER_API_KEY not configured — skipping Rentometer") return None - # Build cache key from inputs - cache_key = ( - f"{address or ''}|{city or ''}|{state or ''}|{zip_code or ''}|{bedrooms}" - ) - - # Check cache - if cache_key in _rent_cache: - ts, cached = _rent_cache[cache_key] - if time.time() - ts < _CACHE_TTL_SECONDS: - if cached and cached.get("median_rent"): - return cast(Decimal, cached["median_rent"]) - return None + cache_key = _cache_key(address, city, state, zip_code, bedrooms) + + cached = cache.get(cache_key) + if cached is not None: + median = cached.get("median_rent") if cached else None + return cast(Decimal, median) if median else None # Make API call if not address or not city or not state or not zip_code: logger.debug("Incomplete address for Rentometer lookup") - _rent_cache[cache_key] = (time.time(), None) + cache.set(cache_key, {}, timeout=_CACHE_TTL_SECONDS) return None try: result = client.get_rent_by_address(address, city, state, zip_code) - _rent_cache[cache_key] = (time.time(), result) + cache.set(cache_key, result or {}, timeout=_CACHE_TTL_SECONDS) if result and result.get("median_rent"): logger.debug( "Rentometer: %s %s, %s %s = $%s", @@ -167,6 +185,6 @@ def get_rent_estimate( return cast(Decimal, result["median_rent"]) except RentometerError as exc: logger.warning("Rentometer lookup failed for %s: %s", zip_code, exc) + cache.set(cache_key, {}, timeout=_CACHE_TTL_SECONDS) - _rent_cache[cache_key] = (time.time(), None) return None diff --git a/core/services/screening.py b/core/services/screening.py index 4beedb2..b6f286d 100644 --- a/core/services/screening.py +++ b/core/services/screening.py @@ -9,10 +9,13 @@ from __future__ import annotations +import logging from dataclasses import dataclass, field from decimal import Decimal from typing import TYPE_CHECKING, Any, Optional, Tuple +logger = logging.getLogger(__name__) + # ── Pure screening evaluator (ported from prei.pipeline.handlers.screening) ── @@ -394,6 +397,7 @@ def _eval_gross_yield( pipeline_property: PipelineProperty, criteria: ScreeningCriteria, source_record: Any | None, + cache_rent: bool = True, ) -> tuple[Decimal, Optional[str], Optional[str]]: """Evaluate gross yield soft criterion. @@ -408,7 +412,7 @@ def _eval_gross_yield( return Decimal("0"), "Gross yield screening skipped — no minimum set", None # Determine if we have rent data - monthly_rent = _get_monthly_rent(pipeline_property, source_record) + monthly_rent = _get_monthly_rent(pipeline_property, source_record, cache_rent) if monthly_rent is None or monthly_rent <= 0: return ( @@ -452,6 +456,7 @@ def _eval_price_to_rent_ratio( pipeline_property: PipelineProperty, criteria: ScreeningCriteria, source_record: Any | None, + cache_rent: bool = True, ) -> tuple[Decimal, Optional[str], Optional[str]]: """Evaluate price-to-rent ratio soft criterion. @@ -467,7 +472,7 @@ def _eval_price_to_rent_ratio( None, ) - monthly_rent = _get_monthly_rent(pipeline_property, source_record) + monthly_rent = _get_monthly_rent(pipeline_property, source_record, cache_rent) if monthly_rent is None or monthly_rent <= 0: return ( @@ -510,9 +515,27 @@ def _eval_price_to_rent_ratio( ) +def _cache_rent(pipeline_property: Any, rent: Decimal, cache_rent: bool) -> Decimal: + """Set estimated_rent on pipeline_property and persist it if possible. + + The DB write is skipped when cache_rent is False (e.g. screening_preview's + documented "without saving" contract) or when pipeline_property has no + `pk` (e.g. a _PipelineView adapted from a HUD/USDA source, which is never + a persisted PipelineProperty). + """ + pipeline_property.estimated_rent = rent + pk = getattr(pipeline_property, "pk", None) + if cache_rent and pk: + from core.models import PipelineProperty as PipelinePropertyModel + + PipelinePropertyModel.objects.filter(pk=pk).update(estimated_rent=rent) + return rent + + def _get_monthly_rent( pipeline_property: PipelineProperty, source_record: Any | None, + cache_rent: bool = True, ) -> Decimal | None: """Get monthly rent from source_record, PipelineProperty, or market APIs. @@ -521,6 +544,11 @@ def _get_monthly_rent( The fallback chain tries real rent data first (Rentometer), then falls back to HUD FMR when Rentometer is unavailable. + + Args: + cache_rent: When False, a fetched rent is still returned but not + persisted to the DB — used by screening_preview(), which promises + not to save anything. """ # 1. Prefer source_record (VRM has actual rent data) if ( @@ -559,14 +587,13 @@ def _get_monthly_rent( bedrooms=int(bedrooms) if bedrooms else 2, ) if rent is not None and rent > 0: - # Cache on pipeline_property for future calls - pipeline_property.estimated_rent = rent - PipelineProperty.objects.filter(pk=pipeline_property.pk).update( - estimated_rent=rent - ) - return rent + return _cache_rent(pipeline_property, rent, cache_rent) except Exception: - pass + logger.warning( + "Rentometer lookup failed for pipeline_property pk=%s", + getattr(pipeline_property, "pk", None), + exc_info=True, + ) # 4. Fallback: HUD Fair Market Rent if zip_code: @@ -579,14 +606,13 @@ def _get_monthly_rent( zip_code=zip_code, bedrooms=int(bedrooms) if bedrooms else 2 ) if rent is not None and rent > 0: - # Cache on pipeline_property for future calls - pipeline_property.estimated_rent = rent - PipelineProperty.objects.filter(pk=pipeline_property.pk).update( - estimated_rent=rent - ) - return rent + return _cache_rent(pipeline_property, rent, cache_rent) except Exception: - pass + logger.warning( + "HUD FMR lookup failed for pipeline_property pk=%s", + getattr(pipeline_property, "pk", None), + exc_info=True, + ) return None @@ -605,7 +631,10 @@ def _extract_zip(source_record: Any | None, pipeline_property: Any) -> str | Non if pipeline_property.address: import re - m = re.search(r"\b(\d{5})\b", pipeline_property.address) + # Anchor to the end (optionally with a ZIP+4 suffix) so a 5-digit + # street number earlier in the address isn't mistaken for the ZIP, + # e.g. "12345 Main St, Austin, TX 78701" must match 78701, not 12345. + m = re.search(r"(\d{5})(?:-\d{4})?\s*$", pipeline_property.address.strip()) if m: return m.group(1) @@ -739,10 +768,16 @@ def _adapt_source_to_pipeline( """ class _PipelineView: + # No `pk` — this is never a persisted PipelineProperty, so callers + # must not assume `.pk` exists (see cache_rent guard in + # _get_monthly_rent). price: Decimal | None = None estimated_rent: Decimal | None = None beds: int | None = None year_built: int | None = None + address: str | None = None + city: str | None = None + state: str | None = None view = _PipelineView() @@ -754,6 +789,12 @@ class _PipelineView: if hasattr(source, "bedrooms") and source.bedrooms is not None: view.beds = int(source.bedrooms) + # Address/city/state so Rentometer's address-based lookup (step 3 of + # _get_monthly_rent) also runs for HUD/USDA sources, not just VRM. + view.address = getattr(source, "address", None) + view.city = getattr(source, "city", None) + view.state = getattr(source, "state", None) + return view @@ -762,6 +803,7 @@ def screen_property( criteria: ScreeningCriteria, source_record: Any | None = None, growth_area: Any | None = None, + cache_rent: bool = True, ) -> ScreeningResult: """Evaluate a PipelineProperty against ScreeningCriteria. @@ -793,6 +835,9 @@ def screen_property( UsdaProperty) being evaluated. criteria: ScreeningCriteria with user's thresholds. source_record: Optional source model instance for additional data. + cache_rent: When False, a Rentometer/HUD FMR rent lookup is + still used for scoring but not persisted to the + DB — pass False from preview/dry-run callers. Returns: ScreeningResult with pass/fail, final score, and diagnostic lists. @@ -905,7 +950,7 @@ def screen_property( # 6. Gross yield ded, pass_msg, fail_msg = _eval_gross_yield( - pipeline_property, criteria, source_record + pipeline_property, criteria, source_record, cache_rent ) score -= ded if pass_msg: @@ -915,7 +960,7 @@ def screen_property( # 7. Price-to-rent ratio ded, pass_msg, fail_msg = _eval_price_to_rent_ratio( - pipeline_property, criteria, source_record + pipeline_property, criteria, source_record, cache_rent ) score -= ded if pass_msg: diff --git a/core/views/__init__.py b/core/views/__init__.py index b8a8b05..5f3c2d6 100644 --- a/core/views/__init__.py +++ b/core/views/__init__.py @@ -2422,7 +2422,9 @@ def screening_preview(request: HttpRequest) -> HttpResponse: for pp in properties: source_record = get_source_record(pp) - result = screen_property(pp, criteria, source_record=source_record) + result = screen_property( + pp, criteria, source_record=source_record, cache_rent=False + ) if result.passed: passed += 1 diff --git a/tests/e2e/test_rentometer_e2e.py b/tests/e2e/test_rentometer_e2e.py index 9363f8c..7d15d64 100644 --- a/tests/e2e/test_rentometer_e2e.py +++ b/tests/e2e/test_rentometer_e2e.py @@ -1,5 +1,7 @@ """E2E tests for Rentometer API integration in screening pipeline.""" +from unittest.mock import patch + import pytest from decimal import Decimal @@ -78,6 +80,101 @@ def test_screening_falls_back_to_hud_fmr(self, rentometer_property) -> None: # In test env without HUD API key, this returns None assert rent is None + def test_extract_zip_prefers_trailing_zip_over_house_number( + self, rentometer_property + ) -> None: + """A 5-digit house number earlier in the address must not be mistaken + for the ZIP when the real ZIP trails the address.""" + rentometer_property.zip_code = "" + rentometer_property.address = "12345 Main St, Austin, TX 78701" + from core.services.screening import _extract_zip + + zip_code = _extract_zip(None, rentometer_property) + assert zip_code == "78701" + + def test_get_monthly_rent_uses_rentometer_when_available( + self, rentometer_property + ) -> None: + """_get_monthly_rent uses and persists a Rentometer estimate when found.""" + from core.services.screening import _get_monthly_rent + + with patch( + "core.integrations.market.rentometer.get_rent_estimate", + return_value=Decimal("1950.00"), + ): + rent = _get_monthly_rent(rentometer_property, None) + + assert rent == Decimal("1950.00") + rentometer_property.refresh_from_db() + assert rentometer_property.estimated_rent == Decimal("1950.00") + + def test_get_monthly_rent_falls_back_to_hud_fmr_when_rentometer_fails( + self, rentometer_property + ) -> None: + """When Rentometer finds nothing, the chain falls through to HUD FMR.""" + from core.services.screening import _get_monthly_rent + + with ( + patch( + "core.integrations.market.rentometer.get_rent_estimate", + return_value=None, + ), + patch( + "core.integrations.market.hud_fmr.get_rent_estimate", + return_value=Decimal("1500.00"), + ), + ): + rent = _get_monthly_rent(rentometer_property, None) + + assert rent == Decimal("1500.00") + rentometer_property.refresh_from_db() + assert rentometer_property.estimated_rent == Decimal("1500.00") + + def test_get_monthly_rent_cache_rent_false_skips_db_write( + self, rentometer_property + ) -> None: + """cache_rent=False (screening_preview) returns the rent but never saves it.""" + from core.services.screening import _get_monthly_rent + + with patch( + "core.integrations.market.rentometer.get_rent_estimate", + return_value=Decimal("1950.00"), + ): + rent = _get_monthly_rent(rentometer_property, None, cache_rent=False) + + assert rent == Decimal("1950.00") + rentometer_property.refresh_from_db() + assert rentometer_property.estimated_rent is None + + def test_get_monthly_rent_never_crashes_for_hud_usda_pipeline_view( + self, growth_area + ) -> None: + """A raw HudProperty (adapted to _PipelineView, no pk) must not crash + _get_monthly_rent even when a rent value is found.""" + from core.models.sources import HudProperty + from core.services.screening import _adapt_source_to_pipeline + + hud_property = HudProperty( + address="456 Oak Ave", + city="Austin", + state="TX", + zip_code="78704", + ) + view = _adapt_source_to_pipeline(hud_property) + assert view.address == "456 Oak Ave" + assert view.city == "Austin" + assert view.state == "TX" + + from core.services.screening import _get_monthly_rent + + with patch( + "core.integrations.market.rentometer.get_rent_estimate", + return_value=Decimal("1700.00"), + ): + rent = _get_monthly_rent(view, hud_property) + + assert rent == Decimal("1700.00") + @staticmethod def _override_settings(**kwargs): """Context manager to override Django settings.""" diff --git a/tests/test_rentometer.py b/tests/test_rentometer.py index 50cadf7..4f36534 100644 --- a/tests/test_rentometer.py +++ b/tests/test_rentometer.py @@ -5,14 +5,26 @@ from decimal import Decimal from unittest.mock import MagicMock, patch +import pytest +import requests +from django.core.cache import cache from core.integrations.market.rentometer import ( RentometerClient, RentometerError, + _parse_rent, get_rent_estimate, ) +@pytest.fixture(autouse=True) +def _clear_rentometer_cache(): + """Prevent cross-test pollution — the cache is process-wide, not per-test.""" + cache.clear() + yield + cache.clear() + + class TestRentometerClient: """Test suite for RentometerClient.""" @@ -154,3 +166,68 @@ def test_caches_rent_estimate(self, MockClient: MagicMock) -> None: assert ( mock_instance.get_rent_by_address.call_count == 1 ) # No additional API call + + +class TestParseRent: + """Edge cases for _parse_rent's Decimal coercion.""" + + def test_none_returns_none(self) -> None: + assert _parse_rent(None) is None + + def test_empty_string_returns_none(self) -> None: + assert _parse_rent("") is None + + def test_non_numeric_returns_none(self) -> None: + assert _parse_rent("not-a-number") is None + + def test_negative_returns_none(self) -> None: + assert _parse_rent(-500) is None + + def test_zero_returns_none(self) -> None: + assert _parse_rent(0) is None + + def test_valid_string_returns_decimal(self) -> None: + assert _parse_rent("1850") == Decimal("1850.00") + + def test_valid_float_returns_decimal(self) -> None: + assert _parse_rent(1850.5) == Decimal("1850.50") + + +class TestGetErrorClassification: + """RentometerClient._get() must funnel every failure through RentometerError.""" + + @patch("core.integrations.market.rentometer.requests.get") + def test_timeout_raises_rentometer_error(self, mock_get: MagicMock) -> None: + mock_get.side_effect = requests.exceptions.Timeout("timed out") + client = RentometerClient(api_key="test-key") + with pytest.raises(RentometerError): + client._get("rent", params={}) + + @patch("core.integrations.market.rentometer.requests.get") + def test_server_error_raises_rentometer_error(self, mock_get: MagicMock) -> None: + mock_resp = MagicMock() + mock_resp.status_code = 500 + mock_resp.raise_for_status.side_effect = requests.exceptions.HTTPError( + "500 Server Error" + ) + mock_get.return_value = mock_resp + client = RentometerClient(api_key="test-key") + with pytest.raises(RentometerError): + client._get("rent", params={}) + + @patch("core.integrations.market.rentometer.requests.get") + def test_server_error_caught_by_get_rent_by_address( + self, mock_get: MagicMock + ) -> None: + """A 500 must not escape as a raw requests.HTTPError past the adapter.""" + mock_resp = MagicMock() + mock_resp.status_code = 500 + mock_resp.raise_for_status.side_effect = requests.exceptions.HTTPError( + "500 Server Error" + ) + mock_get.return_value = mock_resp + client = RentometerClient(api_key="test-key") + result = client.get_rent_by_address( + address="123 Main St", city="Austin", state="TX", zip_code="78701" + ) + assert result is None