Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DISCO-3246] fix: Weather to save normalized city in selected_city field #796

Merged
merged 1 commit into from
Feb 19, 2025
Merged

[DISCO-3246] fix: Weather to save normalized city in selected_city field #796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion merino/providers/suggest/weather/backends/accuweather/backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

language = get_language(weather_context.languages)
region = weather_context.selected_region

Expand Down
10 changes: 8 additions & 2 deletions merino/providers/suggest/weather/backends/accuweather/pathfinder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the following cities since they require not to have regions and there's a bug with that at the moment

("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"]
)
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions merino/providers/suggest/weather/backends/protocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down
90 changes: 84 additions & 6 deletions tests/unit/providers/suggest/weather/backends/test_pathfinder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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(
Expand Down