feat: WestMorlandAndFurness - add postcode + house number lookup#2064
feat: WestMorlandAndFurness - add postcode + house number lookup#2064InertiaUK wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2064 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 9 9
Lines 1141 1141
=======================================
Hits 989 989
Misses 152 152 ☔ View full report in Codecov by Sentry. |
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 28 minutes and 45 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWestmorland & Furness waste collection parser now resolves UPRNs from postcode/house_number input via a new ChangesPostcode-Based UPRN Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
uk_bin_collection/tests/input.json (1)
2719-2721: ⚡ Quick winThis fixture still bypasses the new postcode lookup path.
Because
uprnis still populated,_resolve_uprn(...)returns early and never exercises the postcode + house-number resolution added in this PR. If this entry is meant to cover the new branch, removeuprnhere or add a second postcode-only fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uk_bin_collection/tests/input.json` around lines 2719 - 2721, The fixture with postcode "LA9 4QU" still includes an uprn so _resolve_uprn(...) returns early and never exercises the new postcode + house-number lookup; either remove the "uprn" field from that fixture to force the postcode lookup branch or add a separate postcode-only fixture (same postcode and "house_number": "23" but no uprn) so the postcode+house-number resolution path is exercised by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@uk_bin_collection/uk_bin_collection/councils/WestMorlandAndFurness.py`:
- Around line 48-53: The parse_data method currently extracts paon from kwargs
but not house_number, causing _resolve_uprn to miss address matches; update
parse_data to also read house_number = kwargs.get("house_number") (and/or
fallback to paon if needed) and pass it into the call to _resolve_uprn (e.g.,
_resolve_uprn(user_postcode, uprn=user_uprn, paon=user_paon or house_number) or
by passing a house_number parameter if _resolve_uprn supports it) so the
postcode-based resolution uses the provided house_number from fixtures.
- Around line 57-64: The request to fetch the schedule at URI (the
requests.get(URI) call used before creating soup) lacks a timeout and doesn't
call response.raise_for_status(), so add an explicit timeout argument to
requests.get and immediately call response.raise_for_status() after receiving
the response (and let exceptions propagate or wrap them with a clear message)
before passing response.text to BeautifulSoup; update the code around the URI
variable, the requests.get(URI) invocation, and the creation of soup to include
these safeguards.
- Around line 34-44: The PAON lookup currently uses a broad substring match and
then silently returns options[0][0] on miss; update the logic in
WestMorlandAndFurness.py (the block using paon, paon_norm and options) to only
accept precise matches (e.g., exact token match or startswith with a separator
like " " or "," for text_upper against paon_norm) and remove the loose `if
paon_norm in text.upper()` fallback, and instead raise a clear exception (e.g.,
ValueError) when no option matches rather than returning options[0][0]; ensure
the raised exception includes the paon_norm and available option texts for
debugging.
---
Nitpick comments:
In `@uk_bin_collection/tests/input.json`:
- Around line 2719-2721: The fixture with postcode "LA9 4QU" still includes an
uprn so _resolve_uprn(...) returns early and never exercises the new postcode +
house-number lookup; either remove the "uprn" field from that fixture to force
the postcode lookup branch or add a separate postcode-only fixture (same
postcode and "house_number": "23" but no uprn) so the postcode+house-number
resolution path is exercised by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61789da9-42e2-4ef2-b27d-3e5e4fb9bf67
📒 Files selected for processing (2)
uk_bin_collection/tests/input.jsonuk_bin_collection/uk_bin_collection/councils/WestMorlandAndFurness.py
| if paon: | ||
| paon_norm = str(paon).strip().upper() | ||
| for val, text in options: | ||
| text_upper = text.upper() | ||
| if text_upper.startswith(paon_norm + " ") or text_upper.startswith(paon_norm + ","): | ||
| return val | ||
| for val, text in options: | ||
| if paon_norm in text.upper(): | ||
| return val | ||
|
|
||
| return options[0][0] |
There was a problem hiding this comment.
Don’t silently resolve to a different property when PAON lookup misses.
Line 44 falls back to the first address, and Lines 40-42 also allow a broad substring match. On multi-address postcodes that can return another household’s UPRN instead of failing fast.
Suggested fix
+import re
+
if paon:
paon_norm = str(paon).strip().upper()
for val, text in options:
text_upper = text.upper()
if text_upper.startswith(paon_norm + " ") or text_upper.startswith(paon_norm + ","):
return val
for val, text in options:
- if paon_norm in text.upper():
+ if re.search(rf"(?<!\w){re.escape(paon_norm)}(?!\w)", text.upper()):
return val
+ raise ValueError(f"No address matching '{paon}' for postcode: {postcode}")
return options[0][0]Based on learnings: prefer explicit failures (raise exceptions on unexpected formats) over silent defaults or swallowed errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@uk_bin_collection/uk_bin_collection/councils/WestMorlandAndFurness.py` around
lines 34 - 44, The PAON lookup currently uses a broad substring match and then
silently returns options[0][0] on miss; update the logic in
WestMorlandAndFurness.py (the block using paon, paon_norm and options) to only
accept precise matches (e.g., exact token match or startswith with a separator
like " " or "," for text_upper against paon_norm) and remove the loose `if
paon_norm in text.upper()` fallback, and instead raise a clear exception (e.g.,
ValueError) when no option matches rather than returning options[0][0]; ensure
the raised exception includes the paon_norm and available option texts for
debugging.
UPRN is no longer required. Searches council LocalGov Drupal page by postcode to resolve UPRN from address dropdown. Backward compatible.
InertiaUK
force-pushed
the
feat/westmorland-postcode-lookup
branch
from
c5eafaf to
ab9f683
Compare
1 participant
Summary
How it works
GET /waste-collection-schedule/find?postcode=X→ parse<select name="uprn">→ match by house number in option text → use matched UPRN with existing/view/{uprn}pageTesting
LA9 4QU+ UPRN100110698636✅LA9 4QU+ paon23✅Summary by CodeRabbit