Repository navigation
Conversation
parse_data() ignores the page it's given entirely and makes its own request to a specific, working endpoint - but the generic framework fetch to the bare root URL (online.aberdeenshire.gov.uk/) ran first regardless, and the #2073 retry fix correctly turned that URL's persistent 500 into a hard failure, breaking every user's sensors even though the real data endpoint was healthy the whole time. Override get_data() to skip the unused fetch entirely, so this doesn't depend on skip_get_url being present in already-saved HA config data.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAberdeenshireCouncil adds a class method that skips fetching the supplied root URL and returns an empty string. ChangesAberdeenshire data handling
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2159 +/- ##
=======================================
Coverage 86.89% 86.89%
=======================================
Files 9 9
Lines 1160 1160
=======================================
Hits 1008 1008
Misses 152 152 ☔ View full report in Codecov by Harness. |
Summary
Priority fix for #2158, and one of the 3 new failures found in a fresh full-suite run.
Root cause: this is a self-inflicted regression from the #2073 retry fix (PR #2155, already merged).
AberdeenshireCouncil.parse_data()completely ignores thepageit's given and makes its own request to a specific, working endpoint (/Apps/Waste-Collections/Routes/Route/{uprn}) - but the generic framework fetch to the bare root URL (online.aberdeenshire.gov.uk/, frominput.json'surlfield) ran first regardless, since this council was never markedskip_get_url. That root URL is currently returning a persistent HTTP 500 on the council's own end - previously harmless (a barerequests.get()doesn't raise on non-2xx status), but the #2073 fix's retry session correctly (and now, for this council, unfortunately) turns 5 straight 500s into a hardRetryError, beforeparse_data()(which never needed that page anyway) even runs.Verified live:
online.aberdeenshire.gov.uk/→ 500, butonline.aberdeenshire.gov.uk/Apps/Waste-Collections/Routes/Route/{uprn}→ 200 with valid data, at the same moment.Fix: override
get_data()inAberdeenshireCouncil.pyto skip the fetch entirely (return"") rather than relying onskip_get_url- that flag comes from each user's already-saved HA config data, which existing installs don't have, so setting it ininput.jsonalone would only fix the test suite, not real users. This fix works immediately on upgrade with zero config changes needed. Verified via the actual CLI entrypoint HA uses, with noskip_get_urlflag passed (matching existing users' saved config).Broader finding (not fixed here, flagging for awareness)
Found 59 councils with this same shape -
parse_data()ignorespageand makes its own request, butskip_get_urlisn't set. Most are harmless (theirurlfield happens to point at a healthy resource that's just redundantly fetched), so I haven't touched them. Cross-referenced against a fresh full-suite run: only Aberdeenshire and (initially suspected, then ruled out - see below) Fylde are currently affected. Worth a slower systematic audit at some point, but not urgent since only one is actually live.Test plan
poetry run pytest uk_bin_collection/tests/step_defs/ -k AberdeenshireCouncil- passedcollect_data.py) with noskip_get_urlflag, matching existing users' saved config - returns correct bin datablack --checkcleanFixes #2158
Summary by CodeRabbit