fix(EastDevonDC): zero-pad UPRN to 12 digits#2050
Conversation
East Devon's website requires 12-digit zero-padded UPRNs. Unpadded UPRNs (e.g. from PostGIS lookups) return empty pages. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughEastDevonDC module corrects its dependency imports and adds UPRN normalization. The module now imports ChangesEast Devon UPRN Handling
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2050 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 9 9
Lines 1141 1141
=======================================
Hits 989 989
Misses 152 152 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
uk_bin_collection/uk_bin_collection/councils/EastDevonDC.py (1)
21-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback URL path is broken by unconditional UPRN padding.
Line 24 pads before the fallback check on Line 26, so legacy calls that rely on
urlcan fail early (or never reach fallback). Gate padding behind a presentuprn, then use legacyurlotherwise.💡 Proposed fix
try: user_uprn = kwargs.get("uprn") - check_uprn(user_uprn) - # East Devon requires 12-digit zero-padded UPRNs - user_uprn = user_uprn.zfill(12) - url = f"https://eastdevon.gov.uk/recycling-and-waste/recycling-waste-information/when-is-my-bin-collected/future-collections-calendar/?UPRN={user_uprn}" - if not user_uprn: - # This is a fallback for if the user stored a URL in old system. Ensures backwards compatibility. - url = kwargs.get("url") + if user_uprn: + user_uprn = str(user_uprn) + check_uprn(user_uprn) + # East Devon requires 12-digit zero-padded UPRNs + user_uprn = user_uprn.zfill(12) + url = f"https://eastdevon.gov.uk/recycling-and-waste/recycling-waste-information/when-is-my-bin-collected/future-collections-calendar/?UPRN={user_uprn}" + else: + # This is a fallback for if the user stored a URL in old system. Ensures backwards compatibility. + url = kwargs.get("url") + if not url: + raise ValueError("Missing identifier: provide 'uprn' or 'url'")Based on learnings, parser paths should fail explicitly for invalid identifiers rather than failing indirectly after format assumptions.
🤖 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/EastDevonDC.py` around lines 21 - 28, The code pads and validates user_uprn unconditionally which prevents the legacy url fallback from working; change the logic in the EastDevonDC request flow so you first check whether kwargs contains an "uprn" (user_uprn) and only call check_uprn(user_uprn) and user_uprn = user_uprn.zfill(12) when it is present, otherwise skip padding/validation and set url = kwargs.get("url") as the fallback; keep the final url assignment using the zero-padded user_uprn only in the branch where uprn existed.
🤖 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.
Outside diff comments:
In `@uk_bin_collection/uk_bin_collection/councils/EastDevonDC.py`:
- Around line 21-28: The code pads and validates user_uprn unconditionally which
prevents the legacy url fallback from working; change the logic in the
EastDevonDC request flow so you first check whether kwargs contains an "uprn"
(user_uprn) and only call check_uprn(user_uprn) and user_uprn =
user_uprn.zfill(12) when it is present, otherwise skip padding/validation and
set url = kwargs.get("url") as the fallback; keep the final url assignment using
the zero-padded user_uprn only in the branch where uprn existed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1893fcc6-6f62-496e-bc06-0d7e84016cb5
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/EastDevonDC.py
Summary
.zfill(12)to pad the UPRN before building the URLSummary by CodeRabbit