Skip to content

fix(notifications): isolate AFD discussion fetch from forecast to fix missing update notifications#452

Merged
Orinks merged 3 commits intodevfrom
fix/afd-notification-debug
Mar 14, 2026
Merged

fix(notifications): isolate AFD discussion fetch from forecast to fix missing update notifications#452
Orinks merged 3 commits intodevfrom
fix/afd-notification-debug

Conversation

@Orinks
Copy link
Owner

@Orinks Orinks commented Mar 14, 2026

Problem

AFD (Area Forecast Discussion) update notifications never fired, even though the discussion clearly updated in the UI. Alert notifications worked fine.

Root Cause

get_notification_event_data was calling _get_nws_forecast_and_discussion, which wraps the forecast and discussion fetches in a single try/except. Any transient HTTP error on the forecast endpoint (e.g. 503) caused the entire function to return (None, None, None), silently dropping discussion_issuance_time. Without an issuance time, _check_discussion_update returned None on every poll, so notifications never fired.

Fix

  • weather_client_nws.py: Extracted the forecast fetch into its own try/except inside get_nws_forecast_and_discussion — a forecast failure now logs a warning and returns forecast=None but still returns the discussion. Added a new get_nws_discussion_only() that fetches grid data + AFD without touching the forecast endpoint at all (lighter weight, no blast radius from forecast failures).

  • weather_client_base.py: get_notification_event_data now calls _get_nws_discussion_only instead of _get_nws_forecast_and_discussion, eliminating the unnecessary forecast fetch from the 60-second poll path and removing the silent-suppression bug entirely. Added debug logging for discussion_issuance_time and alert count at each check.

  • notification_event_manager.py: Added debug log lines in _check_discussion_update for every early-return path (no issuance_time, first-run, unchanged, updated) so diagnosing notification failures is straightforward.

Tests

  • New tests/test_nws_afd_notification.py (9 tests):
    • Forecast HTTP 503 → discussion + issuance_time still returned
    • Forecast + discussion both succeed
    • Grid data failure propagates (retryable)
    • get_nws_discussion_only happy path
    • get_nws_discussion_only never calls forecast endpoint
    • get_nws_discussion_only returns (None, None) on non-retryable error
    • get_notification_event_data populates discussion_issuance_time
    • get_notification_event_data calls _get_nws_discussion_only, not forecast+discussion
    • get_notification_event_data doesn't crash on (None, None) from discussion fetch
  • Updated test_split_notification_timers.py to mock _get_nws_discussion_only instead of _get_nws_forecast_and_discussion in the TestGetNotificationEventData class.

🤖 Generated with Claude Code

Orinks and others added 3 commits March 14, 2026 14:39
…ate notifications fire

The notification event path was calling _get_nws_forecast_and_discussion,
which wrapped forecast + discussion in a single try/except. A transient
forecast HTTP error (e.g. 503) caused the whole function to return
(None, None, None), silently dropping discussion_issuance_time and
preventing AFD update notifications from ever firing.

Changes:
- weather_client_nws.py: extract forecast fetch into its own try/except
  inside get_nws_forecast_and_discussion so discussion still returns on
  forecast failure. Add new get_nws_discussion_only() that fetches grid
  data + AFD without touching the forecast endpoint at all.
- weather_client_base.py: get_notification_event_data now calls
  _get_nws_discussion_only instead of _get_nws_forecast_and_discussion,
  removing an unnecessary forecast fetch from the 60-second poll path and
  eliminating the forecast-failure silent suppression entirely. Added
  debug logging for issuance_time and discussion status at each check.
- notification_event_manager.py: add debug log lines in
  _check_discussion_update for every early-return path (no issuance_time,
  first-run, unchanged, and updated) so failures are visible at DEBUG level.
- tests: add tests/test_nws_afd_notification.py with 9 tests covering
  forecast failure isolation, discussion-only happy path, no-forecast-call
  assertion, and get_notification_event_data integration. Update
  test_split_notification_timers.py to mock _get_nws_discussion_only.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…scussion and get_nws_discussion_only

Covers the previously uncovered `client=None` code paths in both NWS
functions (lines 636-643, 650, 657, 709-714, 717, 722, 727) and the
`_get_nws_discussion_only` delegation method in weather_client_base
(line 1066).  diff-cover now reports 100% on all changed lines.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace Linux-only `%-I` format specifier with `%I` + lstrip("0")
to avoid "Invalid format string" errors on Windows.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Orinks Orinks merged commit ababc1e into dev Mar 14, 2026
6 checks passed
@Orinks Orinks deleted the fix/afd-notification-debug branch March 14, 2026 15:44
Orinks added a commit that referenced this pull request Mar 14, 2026
…on (#453)

Improves `summarize_discussion_change` to extract meaningful content
from AFD updates:

1. **Primary**: Extracts `.WHAT HAS CHANGED...` section (NWS includes
this in many AFDs)
2. **Fallback**: Extracts `.KEY MESSAGES...` section
3. **Final fallback**: First new line (existing behavior)

Summary truncated to 300 chars for toast readability.

Follow-up to #452.

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant