From 63f633bc3f5c618ff773ab2a481389d4a4378644 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Tue, 18 Feb 2025 22:04:01 -0800 Subject: [PATCH] [DISCO-3246] fix: Weather to save normalized city in selected_city field --- .../weather/backends/accuweather/backend.py | 2 +- .../backends/accuweather/pathfinder.py | 10 ++- .../suggest/weather/backends/protocol.py | 1 + .../weather/backends/test_pathfinder.py | 90 +++++++++++++++++-- 4 files changed, 94 insertions(+), 9 deletions(-) diff --git a/merino/providers/suggest/weather/backends/accuweather/backend.py b/merino/providers/suggest/weather/backends/accuweather/backend.py index a486b03b5..eaea541dc 100644 --- a/merino/providers/suggest/weather/backends/accuweather/backend.py +++ b/merino/providers/suggest/weather/backends/accuweather/backend.py @@ -546,7 +546,7 @@ async def _fetch_from_cache( """Fetch weather data from cache.""" geolocation = weather_context.geolocation country = geolocation.country - city = geolocation.city + city = weather_context.selected_city language = get_language(weather_context.languages) region = weather_context.selected_region diff --git a/merino/providers/suggest/weather/backends/accuweather/pathfinder.py b/merino/providers/suggest/weather/backends/accuweather/pathfinder.py index fbb23d8ad..0c56d3bdf 100644 --- a/merino/providers/suggest/weather/backends/accuweather/pathfinder.py +++ b/merino/providers/suggest/weather/backends/accuweather/pathfinder.py @@ -11,7 +11,12 @@ MaybeStr = Optional[str] LOCALITY_SUFFIX_PATTERN: re.Pattern = re.compile(r"\s+(city|municipality)$", re.IGNORECASE) -SUCCESSFUL_REGIONS_MAPPING: dict[tuple[str, str], str | None] = {("GB", "London"): "LND"} +SUCCESSFUL_REGIONS_MAPPING: dict[tuple[str, str], str | None] = { + ("GB", "London"): "LND", + ("PH", "Manila"): None, + ("IE", "Dublin"): None, + ("IN", "Hyderabad"): None, +} REGION_MAPPING_EXCLUSIONS: frozenset = frozenset( ["AU", "CA", "CN", "DE", "ES", "FR", "GB", "GR", "IT", "PL", "PT", "RU", "US"] ) @@ -156,10 +161,11 @@ async def explore( if country and city and (country, region, city) in SKIP_CITIES_MAPPING: # increment since we tried to look up this combo again. increment_skip_cities_mapping(country, region, city) + return None, True weather_context.selected_region = region - geolocation.city = city + weather_context.selected_city = city res = await probe(weather_context) if res is not None: diff --git a/merino/providers/suggest/weather/backends/protocol.py b/merino/providers/suggest/weather/backends/protocol.py index d8a27aad4..f1d3621a6 100644 --- a/merino/providers/suggest/weather/backends/protocol.py +++ b/merino/providers/suggest/weather/backends/protocol.py @@ -82,6 +82,7 @@ class WeatherContext: geolocation: Location languages: list[str] selected_region: Optional[str] = None + selected_city: Optional[str] = None distance_calculation: Optional[bool] = None diff --git a/tests/unit/providers/suggest/weather/backends/test_pathfinder.py b/tests/unit/providers/suggest/weather/backends/test_pathfinder.py index e9c3b496a..15c5c6c78 100644 --- a/tests/unit/providers/suggest/weather/backends/test_pathfinder.py +++ b/tests/unit/providers/suggest/weather/backends/test_pathfinder.py @@ -4,7 +4,7 @@ """Unit tests for the Accuweather pathfinder module.""" -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, _Call, call import pytest @@ -66,31 +66,109 @@ def test_compass(location: Location, expected_region_and_city: str) -> None: @pytest.mark.parametrize( - ("weather_context", "expected_probes"), + ("weather_context", "expected_calls"), [ ( WeatherContext( Location(country="CA", regions=["BC"], city="Accènted City"), languages=["en-US"] ), - 3, + [ + call( + WeatherContext( + geolocation=Location( + country="CA", + country_name=None, + regions=["BC"], + region_names=None, + city="Accènted City", + dma=None, + postal_code=None, + key=None, + coordinates=None, + ), + languages=["en-US"], + selected_region="BC", + selected_city="Accented", + distance_calculation=None, + ) + ), + call( + WeatherContext( + geolocation=Location( + country="CA", + country_name=None, + regions=["BC"], + region_names=None, + city="Accènted City", + dma=None, + postal_code=None, + key=None, + coordinates=None, + ), + languages=["en-US"], + selected_region="BC", + selected_city="Accented", + distance_calculation=None, + ) + ), + call( + WeatherContext( + geolocation=Location( + country="CA", + country_name=None, + regions=["BC"], + region_names=None, + city="Accènted City", + dma=None, + postal_code=None, + key=None, + coordinates=None, + ), + languages=["en-US"], + selected_region="BC", + selected_city="Accented", + distance_calculation=None, + ) + ), + ], ), ( WeatherContext( Location(country="CA", regions=["BC"], city="Plain"), languages=["en-US"] ), - 1, + [ + call( + WeatherContext( + geolocation=Location( + country="CA", + country_name=None, + regions=["BC"], + region_names=None, + city="Plain", + dma=None, + postal_code=None, + key=None, + coordinates=None, + ), + languages=["en-US"], + selected_region="BC", + selected_city="Plain", + distance_calculation=None, + ) + ), + ], ), ], ) @pytest.mark.asyncio async def test_explore_uses_all_the_right_city_combos( - weather_context: WeatherContext, expected_probes: int + weather_context: WeatherContext, expected_calls: list[_Call] ) -> None: """Test we try the number of right cities.""" mock_probe = AsyncMock(return_value=None) _ = await explore(weather_context, mock_probe) - assert mock_probe.call_count == expected_probes + assert mock_probe.mock_calls == expected_calls @pytest.mark.parametrize(