Improving performance for councils using SocietyWorks software - #2209
Improving performance for councils using SocietyWorks software#2209dracos wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughSeven council integrations now use ChangesSocietyWorks council integrations
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CouncilClass
participant SocietyWorksClass
participant WasteService
participant ICSCalendar
CouncilClass->>SocietyWorksClass: parse_data(identifier)
SocietyWorksClass->>WasteService: resolve property
WasteService-->>SocietyWorksClass: property identifier
SocietyWorksClass->>WasteService: request ICS calendar
WasteService-->>SocietyWorksClass: ICS response
SocietyWorksClass->>ICSCalendar: parse calendar events
ICSCalendar-->>SocietyWorksClass: sorted collection events
SocietyWorksClass-->>CouncilClass: formatted bin data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
uk_bin_collection/uk_bin_collection/councils/SocietyWorks.py (1)
30-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake redirect suppression per-call.
allow_redirects=Falseis required only for theproperty/{uprn}lookup, which reads theLocationheader. The same setting also applies to thecalendar.icsrequest at Line 85. If a council host redirects that path (canonical trailing slash, scheme upgrade, or session redirect),raise_for_status()accepts the 3xx response and the code then reports "ICS feed returned invalid data", which hides the real cause.♻️ Proposed change
- def _get(self, url): + def _get(self, url, allow_redirects=True): resp = self.session.get( - f"{self.BASE_URL}{url}", allow_redirects=False, timeout=30 + f"{self.BASE_URL}{url}", allow_redirects=allow_redirects, timeout=30 ) return respThen call
self._get(f"property/{uprn}", allow_redirects=False)in_uprn_to_property_idand leave the calendar request with redirects enabled.🤖 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/SocietyWorks.py` around lines 30 - 34, Update _get to accept a per-call allow_redirects option, defaulting to enabled, and pass allow_redirects=False only from _uprn_to_property_id for the property/{uprn} lookup. Keep the calendar.ics call using the default redirect behavior so redirects are followed before validation.wiki/Councils.md (1)
2442-2442: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the documented service URLs with
BASE_URL.Sutton, Merton, and Peterborough document a
/wasteor/waste/suffix. Bexley, Brent, Bromley, and Kingston document the service root, which matches theirBASE_URLvalues. The suffix serves no purpose in the new flow, becauseSocietyWorksClassbuilds every request fromBASE_URL.The Merton URL is also actively harmful:
https://fixmystreet.merton.gov.uk/waste/matches the legacy property-ID regex with an empty capture group. See the comment onuk_bin_collection/uk_bin_collection/councils/SocietyWorks.pyLine 71.Use the service root for all seven councils.
Also applies to: 2550-2550, 3056-3056
🤖 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 `@wiki/Councils.md` at line 2442, Update the documented command URLs for Sutton, Merton, and Peterborough, along with the other four listed councils, to use each service’s root URL without a trailing /waste or /waste/ suffix. Ensure all seven examples align with their corresponding BASE_URL values and avoid the Merton URL form that can match the legacy property-ID regex.
🤖 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/SocietyWorks.py`:
- Around line 41-44: Update the response handling around
resp.headers["Location"] in the relevant request method to explicitly validate
that the Location header exists after resp.raise_for_status(). Raise a clear,
appropriate exception describing the unexpected response format when it is
missing, while preserving the existing property_id extraction for valid redirect
responses.
- Around line 55-61: Update the address-matching loop in the relevant
SocietyWorks lookup method to compare paon_lower only with the leading token of
each option’s normalized text, rather than using a substring test. Track
matching options and return the value only when exactly one option matches;
return None or raise the method’s established explicit failure when no options
or multiple options match, avoiding selection of the first ambiguous result.
- Around line 46-52: Update _address_to_property_id to GET the postcode form
page before submitting the lookup, preserving the session and extracting its
hidden/CSRF fields. POST the collected form data together with postcode, then
continue parsing the response for the address select.
- Around line 71-78: Update the URL handling in the surrounding council
argument-resolution method: guard the regex search when user_url is absent,
require one or more digits in the waste path, and only assign property_id when a
non-empty ID is captured. Preserve the fallback to _uprn_to_property_id for
user_uprn and _address_to_property_id for postcode/PAON inputs, including URLs
ending in waste/.
In `@wiki/Councils.md`:
- Line 640: Replace the leading apostrophe with a hyphen on the UPRN bullet in
wiki/Councils.md at lines 640, 800, 852, 2194, 2448, 2556, and 3062, changing
each entry to a valid Markdown list item.
---
Nitpick comments:
In `@uk_bin_collection/uk_bin_collection/councils/SocietyWorks.py`:
- Around line 30-34: Update _get to accept a per-call allow_redirects option,
defaulting to enabled, and pass allow_redirects=False only from
_uprn_to_property_id for the property/{uprn} lookup. Keep the calendar.ics call
using the default redirect behavior so redirects are followed before validation.
In `@wiki/Councils.md`:
- Line 2442: Update the documented command URLs for Sutton, Merton, and
Peterborough, along with the other four listed councils, to use each service’s
root URL without a trailing /waste or /waste/ suffix. Ensure all seven examples
align with their corresponding BASE_URL values and avoid the Merton URL form
that can match the legacy property-ID regex.
🪄 Autofix
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: 6a539bfe-e1e1-4df7-bd9f-48d337ce4eb2
📒 Files selected for processing (10)
uk_bin_collection/uk_bin_collection/councils/BexleyCouncil.pyuk_bin_collection/uk_bin_collection/councils/BrentCouncil.pyuk_bin_collection/uk_bin_collection/councils/BromleyBoroughCouncil.pyuk_bin_collection/uk_bin_collection/councils/DumfriesandGallowayCouncil.pyuk_bin_collection/uk_bin_collection/councils/KingstonUponThamesCouncil.pyuk_bin_collection/uk_bin_collection/councils/LondonBoroughSutton.pyuk_bin_collection/uk_bin_collection/councils/MertonCouncil.pyuk_bin_collection/uk_bin_collection/councils/PeterboroughCityCouncil.pyuk_bin_collection/uk_bin_collection/councils/SocietyWorks.pywiki/Councils.md
Use the iCal feed, which is more performant, less likely to require update, and contains more information.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2209 +/- ##
=======================================
Coverage 83.30% 83.30%
=======================================
Files 12 12
Lines 1402 1402
=======================================
Hits 1168 1168
Misses 234 234 ☔ View full report in Codecov by Harness. |
|
Ran the full CI suite plus live testing on this — unit/integration tests, HassFest, HACS, and CodeQL all pass. The three red checks (Lint Commit Messages, Validate Release Prerequisites, Parity Check) aren't code problems: Parity Check hit a transient PyPI network timeout unrelated to this diff, and the other two are just commit-message formatting (a couple of subjects end with a period) — not something I'll block on since I handle commit messages at merge time anyway. This is a genuinely nice improvement — replacing the old per-council polling/retry HTML scraping with a shared iCal-feed client is both simpler and much more robust. Verified the new flow live for both the UPRN path (Bexley) and the postcode+address path (Sutton), and all 8 affected councils pass the BDD suite. I did find and fix two real edge cases while testing live, both now folded in on top of your branch:
No CSRF/hidden-token issue in practice, for what it's worth — tested the postcode POST live with nothing but the postcode field and it works fine. Folding this into the September release branch with those two fixes on top, crediting you as the original author. Thanks for a solid contribution! |
Follow-up to robbrad#2209's SocietyWorksClass consolidation - two real bugs found in review, both verified live: - _uprn_to_property_id() accessed resp.headers["Location"] unconditionally after a non-404 response. Confirmed live against Bexley that a valid UPRN does 302 with a Location header - but any response that's neither a 404 nor carries one (a malformed redirect, a stray 200) would raise an opaque KeyError instead of a clear error. Now checks for it and raises a descriptive ValueError. - _address_to_property_id() matched the house name/number as a bare substring anywhere in the option text ("addr_lower in text"). Verified live against Sutton's address list (e.g. "56 Greyhound Road", "16 Greyhound Road") that a paon of "6" would wrongly match one of those before ever reaching a real "6 ..." entry - the same address-matching bug class already fixed for Babergh/Haringey/Slough this cycle. Anchored to the start of the option text instead. Verified both fixes live end-to-end (Sutton via postcode+house number, Bexley via UPRN) and via the BDD suite for all 8 councils this PR moves onto SocietyWorksClass. Co-Authored-By: dracos <matthew@dracos.co.uk>
2 participants
Hi, I'm the Technology Director for SocietyWorks, who provide WasteWorks residential waste service websites to a number of councils. I also run a number of things that have a somewhat similar ethos to this project, e.g. https://traintimes.org.uk or https://postofficeinquiry.dracos.co.uk
I noticed all the councils that we run the bin day page for had slightly different scrapers, all basically doing the same thing but in a number of different ways. We publish an iCal feed on all our bin day pages, which should be easier to use, provide more data than just the next collection, and be more performant than fetching the whole bin day page, which also needs to look up other information. We also provide a UPRN lookup for those places that don't use UPRN directly (for performance reasons).
So I've consolidated all these councils together in one class, that then fetches and uses the iCal feed. I don't think it should affect any current users - it still spots the ID in the URL if provided (Bromley/Kingston), and should now work with either a UPRN or a specific-backend property ID in the UPRN field. Hope that makes sense and is appropriate; happy to answer any questions!
I also spotted Dumfries was fetching its iCal URL twice, so fixed that in a separate commit.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation