feat: EastDevonDC - add postcode + house number lookup via addressfinder#2062
feat: EastDevonDC - add postcode + house number lookup via addressfinder#2062InertiaUK wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 20 minutes and 42 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)
📝 WalkthroughWalkthroughThe East Devon parser is refactored to extract address matching and calendar fetching into helpers, remove the unused pandas import, and change parse_data to resolve UPRNs from postcode/house number via the East Devon addressfinder API before delegating calendar retrieval to the new parser. ChangesEastDevonDC Parser Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 4
🤖 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/EastDevonDC.py`:
- Line 16: Normalize the address UPRN before comparing to the zero-padded target
to avoid false negatives: when checking addr.get("UPRN") against uprn_str in
EastDevonDC, coerce the fetched UPRN to a canonical form (e.g.,
str(addr.get("UPRN","")).zfill(12) or by stripping leading zeros from both
sides) and compare that normalized value to uprn_str so matched addresses are
correctly found.
- Around line 45-46: The code currently returns the empty result `data` when
`calendar_collection` is missing, which hides parsing failures; instead, in the
parsing function where `calendar_collection` is inspected (look for the variable
and surrounding logic in EastDevonDC.py, e.g., the function that builds `data`),
raise an explicit exception (e.g., ValueError or a dedicated ParsingError) with
a clear message like "Missing calendar_collection markup for EastDevonDC" so
upstream code can detect site-format changes; remove the silent `return data`
path and ensure any exception is documented or handled by the caller.
- Around line 118-120: The code currently zero-pads any user_uprn with
str(user_uprn).zfill(12) and proceeds to build the URL even for non-numeric
values; instead validate the UPRN input (variable user_uprn) is numeric and of
appropriate length before padding, and raise a clear exception (e.g.,
ValueError) on invalid input; specifically, add a guard that checks
str(user_uprn).isdigit() (and optionally length constraints) and raise if false,
then apply zfill(12) and build the URL as currently done so only valid numeric
UPRNs trigger the request.
- Around line 35-36: The fetch for the calendar page uses requests.get(url)
without timeout or status checking; update the call to use a timeout (e.g.
timeout=30) and call page.raise_for_status() before creating soup so non-200
responses aren’t parsed as valid HTML; specifically modify the requests.get(url)
call that assigns to page and add page.raise_for_status() prior to
BeautifulSoup(page.text...), mirroring the pattern used elsewhere (timeout=30
and raise_for_status()) and optionally wrap the request in a try/except to
handle RequestException.
🪄 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: 8c37b6e7-0846-425a-b3da-4a21ec11939c
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/EastDevonDC.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2062 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 9 9
Lines 1141 1141
=======================================
Hits 989 989
Misses 152 152 ☔ View full report in Codecov by Sentry. |
UPRN is no longer required. Uses /addressfinder endpoint to resolve postcode to UPRN via house number/name match. Backward compatible.
InertiaUK
force-pushed
the
feat/eastdevon-postcode-lookup
branch
from
951c183 to
b71eb9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/EastDevonDC.py`:
- Around line 100-112: The postcode lookup currently runs even when a UPRN was
supplied and then overwrites user_uprn with a fallback; change the logic so the
postcode/addressfinder block only runs when no UPRN was provided (i.e., guard it
with a check like "if user_postcode and not user_uprn") or, alternatively, after
calling _match_address only assign to user_uprn when the original user_uprn was
falsy; update the block around user_postcode, _match_address, matched and the
assignment to user_uprn to preserve an explicitly supplied UPRN.
🪄 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: a7668374-f5a2-4e4b-940d-173bac4a0d30
📒 Files selected for processing (2)
uk_bin_collection/tests/input.jsonuk_bin_collection/uk_bin_collection/councils/EastDevonDC.py
🚧 Files skipped from review as they are similar to previous changes (1)
- uk_bin_collection/tests/input.json
1 participant
Summary
s/addressfinder?qtype=bins&term={postcode}` endpoint which returns addresses with UPRNs. Matches by house number/name, then uses the resolved UPRN with the existing calendar page.How it works
GET /addressfinder?qtype=bins&term={postcode}→ JSON with addresses + UPRNslabelfield?UPRN=Xcalendar pageTesting
010000273781✅EX10 8DP+ paonEGYPT✅Summary by CodeRabbit
New Features
Bug Fixes
Chores