fix(screening): address code-review findings on Rentometer fallback chain

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

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:

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