From ee0b9466d45b6732f2ccc9e7d742c483ff95b896 Mon Sep 17 00:00:00 2001 From: Nitzan Pomerantz <9297302+nitzpo@users.noreply.github.com> Date: Sat, 25 Oct 2025 00:08:05 +0300 Subject: [PATCH] Fix: Critical filtering bugs - missing data handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- nadlan_mcp/govmap.py | 28 +++++++---- tests/test_govmap_client.py | 99 +++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 9 deletions(-) diff --git a/nadlan_mcp/govmap.py b/nadlan_mcp/govmap.py index 0a02c9e..8052c5c 100644 --- a/nadlan_mcp/govmap.py +++ b/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 - rooms = deal.get("assetRoomNum") - if rooms is not None: + if min_rooms is not None or max_rooms is not None: + rooms = deal.get("assetRoomNum") + 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 - price = deal.get("dealAmount") - if price is not None: + if min_price is not None or max_price is not None: + price = deal.get("dealAmount") + 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 - area = deal.get("assetArea") - if area is not None: + if min_area is not None or max_area is not None: + area = deal.get("assetArea") + 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", "") diff --git a/tests/test_govmap_client.py b/tests/test_govmap_client.py index f0ee613..9e75675 100644 --- a/tests/test_govmap_client.py +++ b/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