Refactor BedfordshireCouncil data parsing logic - #2186
Conversation
Bedfordshire website redesign required a new scraper. The current website (as of 22nd July) seems to be a bit of a mess with how the data is reported and has some duplicates. This scraper takes that into account, but may just be a temporary fix! I've maintained original names prior to this update
📝 WalkthroughWalkthroughBedfordshire collection parsing now retrieves data from a council view URL using the UPRN, handles the updated HTML structure, maps collection types by keywords, and removes duplicate date/type entries. ChangesBedfordshire parser
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ParseData
participant CouncilView
participant CollectionEntries
ParseData->>CouncilView: GET view URL with uprn
CouncilView-->>ParseData: Return collection HTML
ParseData->>CollectionEntries: Map dates and bin types
ParseData->>CollectionEntries: Append deduplicated entries
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 #2186 +/- ##
=======================================
Coverage 86.89% 86.89%
=======================================
Files 9 9
Lines 1160 1160
=======================================
Hits 1008 1008
Misses 152 152 ☔ View full report in Codecov by Harness. |
![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
🤖 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/BedfordshireCouncil.py`:
- Around line 14-18: Update BedfordshireCouncil.parse_data to remove the
unconditional check_postcode(user_postcode) call, since the fetch flow only
requires uprn; retain check_uprn(user_uprn) and leave postcode retrieval
unchanged unless it is explicitly needed elsewhere.
- Around line 20-26: Update the council request in the relevant data-fetch
method to use a finite timeout and enable SSL certificate verification by
removing verify=False. Remove requests.packages.urllib3.disable_warnings(), and
if warning suppression is still required, use the direct
urllib3.disable_warnings(...) approach established by
AbstractGetBinDataClass.get_data. Preserve the hardcoded host and existing
non-200 error handling.
- Around line 32-58: Update the parsing loop around the date_element and
type_element checks to raise an explicit error when the expected <time datetime>
element or waste-collection type span is missing or malformed, instead of
continuing. After normalizing raw_type, validate that it contains at least one
recognized bin label; raise explicitly for unrecognized labels before appending
extracted_bins, while preserving the existing mappings for valid labels.
🪄 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: ec0a659a-feef-46e6-a16d-7950cf5c0206
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/BedfordshireCouncil.py
Fix parsing for Central Bedfordshire website update. Fixes robbrad#2185
There was a problem hiding this comment.
Actionable comments posted: 1
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/BedfordshireCouncil.py (1)
32-32: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not silently accept missing collection rows.
When Line 32 finds no matching elements, the method returns an apparently valid empty schedule. Detect an explicit “no collections” response; otherwise raise a clear parsing error so selector changes are noticed.
🤖 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/BedfordshireCouncil.py` at line 32, Update the method containing the collection_days lookup to distinguish an explicit “no collections” response from an unexpected empty result. If the page clearly indicates no collections, preserve the empty schedule behavior; otherwise raise a clear parsing error when soup.find_all returns no matching waste-collection__day elements.Source: Learnings
🧹 Nitpick comments (1)
uk_bin_collection/uk_bin_collection/councils/BedfordshireCouncil.py (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the wildcard import with explicit names.
Line 5 triggers Ruff F403 and hides this module’s dependencies. Import
check_uprnanddate_formatexplicitly.Proposed diff
-from uk_bin_collection.uk_bin_collection.common import * +from uk_bin_collection.uk_bin_collection.common import check_uprn, date_format🤖 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/BedfordshireCouncil.py` at line 5, Replace the wildcard import in BedfordshireCouncil with an explicit import of check_uprn and date_format from the common module, preserving the module’s existing behavior while resolving the hidden-dependency warning.Source: Linters/SAST tools
🤖 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/BedfordshireCouncil.py`:
- Around line 15-23: The parse_data method currently performs redundant network
I/O and ignores its page argument. Move the UPRN-based request into the class’s
retrieval hook used by AbstractGetBinDataClass.get_and_parse_data, ensuring
skip_get_url remains respected, and update parse_data to parse the
already-fetched page response without issuing another request.
---
Outside diff comments:
In `@uk_bin_collection/uk_bin_collection/councils/BedfordshireCouncil.py`:
- Line 32: Update the method containing the collection_days lookup to
distinguish an explicit “no collections” response from an unexpected empty
result. If the page clearly indicates no collections, preserve the empty
schedule behavior; otherwise raise a clear parsing error when soup.find_all
returns no matching waste-collection__day elements.
---
Nitpick comments:
In `@uk_bin_collection/uk_bin_collection/councils/BedfordshireCouncil.py`:
- Line 5: Replace the wildcard import in BedfordshireCouncil with an explicit
import of check_uprn and date_format from the common module, preserving the
module’s existing behavior while resolving the hidden-dependency warning.
🪄 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: ee858e68-4d61-4b8b-a1be-15e3411e62f0
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/BedfordshireCouncil.py
| def parse_data(self, page: str, **kwargs) -> dict: | ||
| user_uprn = kwargs.get("uprn") | ||
| user_postcode = kwargs.get("postcode") | ||
|
|
||
|
|
||
| check_uprn(user_uprn) | ||
| check_postcode(user_postcode) | ||
|
|
||
| # Start a new session to walk through the form | ||
| requests.packages.urllib3.disable_warnings() | ||
| s = requests.Session() | ||
|
|
||
| headers = { | ||
| "Origin": "https://www.centralbedfordshire.gov.uk", | ||
| "Referer": "https://www.centralbedfordshire.gov.uk/info/163/bins_and_waste_collections_-_check_bin_collection_day", | ||
| api_url = f"https://www.centralbedfordshire.gov.uk/waste-and-recycling/waste-collection-schedule/view/{user_uprn}" | ||
|
|
||
| urllib3.disable_warnings(category=urllib3.exceptions.InsecureRequestWarning) | ||
| response = requests.get(api_url, timeout=30) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep network I/O out of parse_data.
AbstractGetBinDataClass.get_and_parse_data already fetches address_url and passes the result as page. This implementation ignores page, performs a second request, and defeats skip_get_url. Move the UPRN URL fetch into the retrieval hook, then parse the fetched response once.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 22-22: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(api_url, timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🤖 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/BedfordshireCouncil.py` around
lines 15 - 23, The parse_data method currently performs redundant network I/O
and ignores its page argument. Move the UPRN-based request into the class’s
retrieval hook used by AbstractGetBinDataClass.get_and_parse_data, ensuring
skip_get_url remains respected, and update parse_data to parse the
already-fetched page response without issuing another request.
|
Thanks for the rewrite — the council's site redesign definitely needed this. Folded into the consolidated "July Part 2" release in #2189, credited to you there. I rewrote the commit history slightly (squashed to one conventional-commit message) since the original commit message was failing the "Lint Commit Messages" check, but the parser logic is yours unchanged (plus a Generated by Claude Code |
Central Bedfordshire's site changed to a new waste-collection-schedule page layout with duplicated entries. Re-derive bins from the new li.waste-collection__day markup, mapping raw type strings back to the original bin names, and de-duplicate repeated entries. Fixes robbrad#2185. Adapted from PR robbrad#2186 by ChadH360.
2 participants
Bedfordshire website redesign required a new scraper. The current website (as of 22nd July) seems to be a bit of a mess with how the data is reported and has some duplicates. This scraper takes that into account, but may just be a temporary fix! I've maintained original names prior to this update
Fixes #2185
Summary by CodeRabbit