fix(broxtowe): rewrite scraper for the council's rebuilt self-service form - #2190
Merged
Conversation
The scraper hardcoded exact AchieveForms field ids like ctl00_ContentPlaceHolder1_FF5683TB. Those numeric ids are reassigned by the form builder whenever the council edits and republishes the form, which is the most likely cause of the reported NoSuchElementException on the postcode textbox. Match each field by its stable role suffix (TB/BTN/DDL/FormGroup) under the same id prefix instead, so a future republish that only renumbers fields doesn't break this again. Raises a clear ValueError naming the expected role if the match isn't exactly one element, rather than a bare Selenium exception. Caveat: no network access to the live Broxtowe form from this environment, so this is verified only against synthetic fixtures for the lookup logic itself (test_broxtowe_borough_council.py), not end-to-end against the real site. Relates to #2188 - needs a live confirmation before it can be considered resolved.
Contributor
📝 WalkthroughWalkthroughBroxtowe form parsing now finds fields by stable role-based ID patterns rather than fixed numeric IDs. The change adds strict lookup validation, updates postcode, search, address, and results handling, and adds tests for successful, missing, and ambiguous matches. ChangesBroxtowe form parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2190 +/- ##
==========================================
- Coverage 83.23% 80.04% -3.19%
==========================================
Files 11 12 +1
Lines 1306 1393 +87
==========================================
+ Hits 1087 1115 +28
- Misses 219 278 +59 ☔ View full report in Codecov by Harness. |
Owner
Author
|
CI's live integration test (which runs against the real site via the "Found 0" means the live page no longer has anything matching the This PR's fallback behavior (raising a clear error instead of a bare Selenium exception) is still a real improvement and I'll leave it in, but it does not resolve #2188 — the current live form needs to actually be inspected (view-source or dev tools) to find out what markup replaced it, and I have no way to do that from here. Flagging as unresolved rather than claiming a fix; would need someone with real browser access to grab the current form's HTML before this can go further. Generated by Claude Code |
…e missing CI's own live integration test (which runs against the real site from a GitHub-hosted runner, unlike this session's network) just proved the suffix-matching fix doesn't find anything at all under the old ctl00_ContentPlaceHolder1_FF* prefix - not a renumbering, the whole id scheme is gone from the page. There's no way to inspect the live form from this session directly, so add a temporary diagnostic dump (current URL, title, and every input/select/button/textarea/form element's tag/id/name/type/class) that prints to stdout right before the lookup raises. CI will capture it in the test's "Captured stdout call" section, which is the only way left to see what actually replaced the old form. Diagnostic only - not covered by new tests, and should be removed once a real fix lands from whatever this reveals.
robbrad
force-pushed
the
claude/open-issues-rkmshu
branch
from
July 25, 2026 23:41
80d8ad0 to
b5d9ae7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/BroxtoweBoroughCouncil.py (1)
20-29: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe selector still targets a prefix known to be absent on the live form.
FIELD_ID_PREFIXremains hard-coded toctl00_ContentPlaceHolder1_FF, but live verification confirms the current form no longer emits that prefix. Every lookup therefore finds zero elements and raises before the scraper can proceed. Inspect the live markup and base the selector on its replacement stable attribute or structure; suffix matching alone does not fix this form.🤖 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/BroxtoweBoroughCouncil.py` around lines 20 - 29, Update _find_field_by_role and FIELD_ID_PREFIX so field lookup uses the stable attribute or DOM structure present in the live form instead of the obsolete ctl00_ContentPlaceHolder1_FF prefix. Preserve role-suffix matching where applicable, but ensure selectors match the current form’s actual field elements and no longer depend on the absent prefix.
🧹 Nitpick comments (1)
uk_bin_collection/uk_bin_collection/councils/BroxtoweBoroughCouncil.py (1)
73-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid catching every exception in the diagnostic fallback.
except Exceptionhides unexpected Selenium or programming errors as a print-only message. Catch the specific expected driver/lookup failures, or preserve the traceback when logging. Based on static analysis, this is a blind exception catch.🤖 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/BroxtoweBoroughCouncil.py` around lines 73 - 74, Update the diagnostic fallback around the page-element enumeration to avoid the broad except Exception handler: catch only the expected Selenium or lookup exception types, and preserve traceback details through the existing diagnostic logging mechanism when reporting them.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/BroxtoweBoroughCouncil.py`:
- Around line 60-62: Update the diagnostic element collection in the relevant
BroxtoweBoroughCouncil flow to include identified div elements, such as by
extending the selector used by driver.find_elements with div[id]. Preserve the
existing input, select, button, textarea, and form selectors so failed lookups
also dump the FormGroup results container.
---
Outside diff comments:
In `@uk_bin_collection/uk_bin_collection/councils/BroxtoweBoroughCouncil.py`:
- Around line 20-29: Update _find_field_by_role and FIELD_ID_PREFIX so field
lookup uses the stable attribute or DOM structure present in the live form
instead of the obsolete ctl00_ContentPlaceHolder1_FF prefix. Preserve
role-suffix matching where applicable, but ensure selectors match the current
form’s actual field elements and no longer depend on the absent prefix.
---
Nitpick comments:
In `@uk_bin_collection/uk_bin_collection/councils/BroxtoweBoroughCouncil.py`:
- Around line 73-74: Update the diagnostic fallback around the page-element
enumeration to avoid the broad except Exception handler: catch only the expected
Selenium or lookup exception types, and preserve traceback details through the
existing diagnostic logging mechanism when reporting them.
🪄 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 Plus
Run ID: 6badd2f9-3ddb-4428-9f81-c28aec526502
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/BroxtoweBoroughCouncil.py
…he rest CI's live diagnostic dump revealed the actual current page: the council rebuilt this form on a new platform (renderform.aspx now redirects to renderform, no more ctl00_ContentPlaceHolder1_* ASP.NET WebForms ids). The field number (5683) survived the rebuild, only the id scheme changed - it's now semantic: FF5683-text (postcode input), FF5683-find (search button), FF5683-list (address dropdown). Switch to those real ids for the first three steps. The dropdown's option value format and the post-submit results container id are still unknown, so: - dump every dropdown option's value/text before attempting to match, since the old "U"+UPRN value assumption was specific to the old page - attempt matching by house-number text (should still work, address text formatting is unlikely to have changed) with UPRN as a best-effort fallback - deliberately dump the page and stop right after clicking the new submit-button id, since the results container's real id isn't known yet either Removed the speculative role-suffix-matching fallback from the previous commit along with its tests - that hypothesis (renumbered fields, same scheme) is now known wrong, so keeping it would be misleading dead code.
Live CI run showed "dropdown has 0 options" - the <select id="FF5683-list"> renders immediately but its <option> entries are filled in asynchronously after the search click, so waiting only for the element's presence raced ahead of the real data. Wait for more than one option (the placeholder plus at least one real address) before reading it.
…t's SPA transition Live CI dump revealed the dropdown's real option value format: "U<uprn>|<full address text>" (e.g. "U100031320105|2 High Street, Kimberley, NOTTINGHAM, NG16 2LS"), not the old page's "U"+UPRN alone. Match on the value's UPRN prefix directly instead of exact select_by_value, with the house-number text match as fallback - both now confirmed working against the live form in CI (the fallback matched and got the flow to the submit step). Clicking submit-button doesn't navigate to a new URL - this form re-renders its section via JS, so the previous section's elements (e.g. the dropdown) go stale instead. The diagnostic dump run after submit hit "stale element reference" because it read the DOM before that transition finished. Wait for the dropdown to go stale first.
…section Live CI got past address selection and submit for the first time, landing on a "Bin Details" section (new URL pattern: RenderForm?g=...&s=...) with an unexplained visible text field FF5691 and the same submit-button id reused. Element attributes alone don't say what FF5691 is asking for - dump the form's rendered text (labels, instructions) to find out before guessing whether it needs to be filled in.
… form Four rounds of live diagnostics against the real site (run through CI's Selenium job, since this environment's network can't reach it directly) established what actually changed: - renderform.aspx now redirects to renderform; the old ctl00_ContentPlaceHolder1_FF5683* ASP.NET WebForms ids are gone entirely, replaced with semantic ids (FF5683-text/-find/-list). The field number (5683) survived the rebuild, only the id scheme did not. - The address dropdown renders before its options populate asynchronously after the search click - waiting only for the <select>'s presence raced ahead of the real data. - Each option's value is "U<uprn>|<full address text>", not the old page's "U"+UPRN alone. - Submitting the address selection re-renders the section via JS rather than navigating to a new URL, landing directly on a "Bin Details" section that already contains the results table - no further interaction needed. Its shape (Bin Type / Collection Day / Last Collection / Next Collection, "%A, %d %B %Y" dates) happens to match what the old page used, so that half of the original parsing logic carries over unchanged. Rewrites the Selenium flow against the real ids/timing and extracts table parsing into _parse_collection_table for unit coverage. Verified end-to-end against the real site via CI's live integration test (BroxtoweBoroughCouncil-scoped Selenium job) reaching the results table successfully with the repo's test UPRN/postcode. Fixes #2188.
robbrad
changed the title
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
2 participants
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.
Summary
Fixes #2188 — Broxtowe's scraper was hitting
NoSuchElementExceptionon the postcode textbox (ctl00_ContentPlaceHolder1_FF5683TB).Root cause: the council rebuilt this form on a new platform.
renderform.aspxnow redirects torenderform, and the old ASP.NET WebForms ids (ctl00_ContentPlaceHolder1_FF5683*) are gone entirely, replaced with semantic ids. The field number (5683) survived the rebuild, only the id scheme changed.I don't have network access to the live site from this sandbox (outbound proxy explicitly denies
selfservice.broxtowe.gov.uk), so I used this repo's ownbehave_pull_request.ymlCI job — which runs a real Selenium session against the live site, scoped to just the changed council, from a GitHub-hosted runner — as a diagnostic channel. Four rounds of pushing temporary diagnostic dumps and reading the results back from CI logs established:#FF5683-text, search button:#FF5683-find, address dropdown:#FF5683-list<select>'s presence raced ahead of the real data"U<uprn>|<full address text>", not the old page's"U"+UPRNaloneBin Type / Collection Day / Last Collection / Next Collection, dates as"%A, %d %B %Y") happens to match what the old page used, so that half of the original parsing logic carries overVerification
This is confirmed working end-to-end against the real live site, not a guess: CI's live integration test (
Run Integration Tests, Selenium session againstselfservice.broxtowe.gov.uk, scoped toBroxtoweBoroughCouncilvia the repo's test UPRN/postcode) now passes:That scenario covers scraping, JSON validity, and schema validation of the actual returned bin data.
Test plan
test_broxtowe_borough_council.py): extraction, empty-date skip, missing-table case — all passblack --checkclean