Fix: Critical filtering bugs - missing data handling

Fixed two critical bugs in filter_deals_by_criteria():

Bug #1: CRASH on None property type
- Issue: Code called .lower() on None when propertyTypeDescription was null
- Impact: Would crash MCP tool in production with incomplete data
- Fix: Check if deal_type is None/empty before calling .lower()

Bug #2: Missing data passes through filters
- Issue: Deals with None/missing values passed filters when they shouldn't
- Example: Filtering by area=60-70 would include deals with assetArea=None
- Impact: get_valuation_comparables returned inflated results with bad data
- Fix: Explicitly check for None when filter is active and exclude those deals

Changes:
- Property type filter: Check for None/empty before normalization
- Area filter: Exclude deals with missing area when min/max_area specified
- Room filter: Exclude deals with missing rooms when min/max_rooms specified
- Price filter: Exclude deals with missing price when min/max_price specified
- Invalid data: Changed from 'pass' to 'continue' to exclude bad data

Added 6 comprehensive unit tests:
- test_filter_excludes_missing_property_type
- test_filter_excludes_missing_area
- test_filter_excludes_missing_rooms
- test_filter_excludes_missing_price
- test_filter_excludes_invalid_numeric_data
- test_filter_allows_missing_data_when_no_filter

This fixes the E2E issue where get_valuation_comparables was returning
incomplete results. Now filters properly exclude deals with missing data.

Test results: 34/34 passing (6 new tests added)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

nadlan_mcp/govmap.py

@@ -842,6 +842,10 @@ class GovmapClient:
                 deal_type = deal.get(
                     "propertyTypeDescription", deal.get("assetTypeHeb", "")
                 )
+                # Skip deals with missing property type data when filter is active
+                if not deal_type:
+                    continue
+
                 # Normalize both strings for flexible matching
                 property_type_normalized = property_type.lower().strip()
                 deal_type_normalized = deal_type.lower().strip()
@@ -852,8 +856,10 @@ class GovmapClient:
                     continue
 
             # Room count filter
+            if min_rooms is not None or max_rooms is not None:
                 rooms = deal.get("assetRoomNum")
-            if rooms is not None:
+                if rooms is None:
+                    continue  # Skip deals with missing room data when filter is active
                 try:
                     rooms = float(rooms)
                     if min_rooms is not None and rooms < min_rooms:
@@ -861,11 +867,13 @@ class GovmapClient:
                     if max_rooms is not None and rooms > max_rooms:
                         continue
                 except (TypeError, ValueError):
-                    pass  # Skip deals with invalid room data
+                    continue  # Skip deals with invalid room data when filter is active
 
             # Price filter
+            if min_price is not None or max_price is not None:
                 price = deal.get("dealAmount")
-            if price is not None:
+                if price is None:
+                    continue  # Skip deals with missing price data when filter is active
                 try:
                     price = float(price)
                     if min_price is not None and price < min_price:
@@ -873,11 +881,13 @@ class GovmapClient:
                     if max_price is not None and price > max_price:
                         continue
                 except (TypeError, ValueError):
-                    pass  # Skip deals with invalid price data
+                    continue  # Skip deals with invalid price data when filter is active
 
             # Area filter
+            if min_area is not None or max_area is not None:
                 area = deal.get("assetArea")
-            if area is not None:
+                if area is None:
+                    continue  # Skip deals with missing area data when filter is active
                 try:
                     area = float(area)
                     if min_area is not None and area < min_area:
@@ -885,7 +895,7 @@ class GovmapClient:
                     if max_area is not None and area > max_area:
                         continue
                 except (TypeError, ValueError):
-                    pass  # Skip deals with invalid area data
+                    continue  # Skip deals with invalid area data when filter is active
 
             # Floor filter
             floor_str = deal.get("floorNo", "")

tests/test_govmap_client.py

@@ -551,3 +551,102 @@ class TestMarketAnalysisFunctions:
         # Deal from different street (should not match)
         deal_address_other_street = "בילינסון 6"
         assert client._is_same_building(search_address, deal_address_other_street) is False
+
+    def test_filter_excludes_missing_property_type(self):
+        """Test that deals with missing property type are excluded when filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "propertyTypeDescription": "דירה"},
+            {"dealId": "2", "propertyTypeDescription": None},
+            {"dealId": "3", "propertyTypeDescription": "בית"},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, property_type="דירה")
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_missing_area(self):
+        """Test that deals with missing area are excluded when area filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "assetArea": 65},
+            {"dealId": "2", "assetArea": None},
+            {"dealId": "3", "assetArea": 50},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, min_area=60, max_area=70)
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_missing_rooms(self):
+        """Test that deals with missing room count are excluded when room filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "assetRoomNum": 3},
+            {"dealId": "2", "assetRoomNum": None},
+            {"dealId": "3", "assetRoomNum": 2},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, min_rooms=2.5, max_rooms=4)
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_missing_price(self):
+        """Test that deals with missing price are excluded when price filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "dealAmount": 2000000},
+            {"dealId": "2", "dealAmount": None},
+            {"dealId": "3", "dealAmount": 1500000},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, min_price=1800000, max_price=2200000)
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_invalid_numeric_data(self):
+        """Test that deals with invalid numeric data are excluded when filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "assetArea": 65, "assetRoomNum": 3, "dealAmount": 2000000},
+            {"dealId": "2", "assetArea": "invalid", "assetRoomNum": 3, "dealAmount": 2000000},
+            {"dealId": "3", "assetArea": 65, "assetRoomNum": "bad", "dealAmount": 2000000},
+            {"dealId": "4", "assetArea": 65, "assetRoomNum": 3, "dealAmount": "wrong"},
+        ]
+
+        # Area filter should exclude deal 2
+        filtered_area = client.filter_deals_by_criteria(deals, min_area=60, max_area=70)
+        assert len(filtered_area) == 3
+        assert all(d["dealId"] in ["1", "3", "4"] for d in filtered_area)
+
+        # Room filter should exclude deal 3
+        filtered_rooms = client.filter_deals_by_criteria(deals, min_rooms=2, max_rooms=4)
+        assert len(filtered_rooms) == 3
+        assert all(d["dealId"] in ["1", "2", "4"] for d in filtered_rooms)
+
+        # Price filter should exclude deal 4
+        filtered_price = client.filter_deals_by_criteria(deals, min_price=1500000, max_price=2500000)
+        assert len(filtered_price) == 3
+        assert all(d["dealId"] in ["1", "2", "3"] for d in filtered_price)
+
+    def test_filter_allows_missing_data_when_no_filter(self):
+        """Test that deals with missing data pass through when no filter is active for that field."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "propertyTypeDescription": "דירה", "assetArea": 65},
+            {"dealId": "2", "propertyTypeDescription": "דירה", "assetArea": None},
+            {"dealId": "3", "propertyTypeDescription": "דירה"},  # Missing assetArea entirely
+        ]
+
+        # Filter by property type only - missing area should pass through
+        filtered = client.filter_deals_by_criteria(deals, property_type="דירה")
+
+        assert len(filtered) == 3  # All should pass since we're not filtering by area