Repository navigation
fix: restore the stored-URL fallback that 0.173.5 broke for 8 councils - #2344
Merged
Merged
Conversation
Cheshire East, Dartford, Eastbourne, Glasgow, Herefordshire, Lewes, Liverpool and Newham accept the UPRN either as `uprn` or inside a URL stored by an old Home Assistant config (no `uprn` field at all). Each called check_uprn() first and fell back to the URL afterwards. Until 0.173.5 check_uprn() only printed a warning for a missing UPRN, so the fallback ran; #2291 made it raise, so every legacy install now fails at setup with "Invalid UPRN" (reported in #2294 for Cheshire East and Liverpool, which worked for over a year before the update). Follow Huntingdon's existing structure: use the UPRN when given, else the stored URL, and only raise the clear "Invalid UPRN" error when neither exists. check_uprn() itself stays strict, as #2291 intended, for the ~180 scrapers where a UPRN is genuinely required. A parametrized test covers all eight (legacy URL only, UPRN wins over URL, neither is a clear error). Against the unfixed scrapers the 8 legacy cases fail and the other 16 pass. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015MaCS29izthZp2dwtvLwf4
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 45 minutes. View limit details
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. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a regression from 0.173.5, and the most important item in this release
#2294: "This has been working for me for over a year until updating last night to 0.173.5" (Cheshire East), with a second user reporting the same for Liverpool.
Cause
Eight scrapers accept the UPRN either as
uprnor inside a URL stored by an old Home Assistant config (those configs have nouprnfield at all):Until 0.173.5,
check_uprn()only printed a warning for a missing UPRN and returned, so the fallback ran. #2291 made it raise (the right call for scrapers that genuinely need a UPRN), but these eight called it before the fallback. So every legacy install fails at setup with exactly the message in the issue:Error getting identifier: Invalid UPRN. Please check the provided UPRN.Affected: Cheshire East, Dartford, Eastbourne, Glasgow, Herefordshire, Lewes, Liverpool, Newham.
How I found the full set
Not just the two reported. I scanned all 190
check_uprn/check_usrncall sites for the "check, then fall back to the URL" shape and cross-checked with every scraper that readskwargs["url"]. That gave these 8 (plus Charnwood, which has the same bug and is fixed by contributor PR #2323 in the November release PR). The other hits were false positives: variables computed internally, orurlused as the page to open rather than a stored UPRN link. Huntingdon already had the correct structure and is the model I followed.Fix
Use the UPRN when given, else the stored URL, and raise the clear
Invalid UPRNerror only when neither exists:check_uprn()itself stays strict, as #2291 intended, for the ~180 scrapers where a UPRN really is required.This touches 8 scraper files plus one test file. That's over the usual 3-file guideline, but it's one regression and the same 5-line change in each, so splitting it would only multiply releases.
Tests
test_legacy_url_fallback.pyis parametrized over all eight and covers three cases each: only a legacy URL (also withuprn=Noneanduprn=""), a UPRN winning over a stored URL, and neither (a clear error, with no request made).I checked the tests detect the bug: run against the unfixed scrapers from master, exactly the 8 legacy-URL cases fail (one per council) and the other 16 pass.
Test plan
make unit-testson Python 3.14.10: 519 passed. The 7 failures are the same as master's: 3 sandbox-only network tests, plus 4 already fixed by test: stop the Orkney and Broxbourne tests failing on the calendar and the network #2341.lint.yml.uprn. That exercises the normal path but not the legacy path, which needs a real stored-URL config. Whoever can reproduce (the Error getting identifier: Invalid UPRN. Please check the provided UPRN. #2294 reporters) should confirm after release.This is a
fix:, so merging it triggers a release.Targets #2294. I'm deliberately not using
Fixes: the logic is covered by tests, but only a real legacy-URL install can confirm it, so the issue should stay open until a reporter does.Generated by Claude Code