fix(NewarkAndSherwoodDC): Add postcode search to resolve correct UPRN#2056
fix(NewarkAndSherwoodDC): Add postcode search to resolve correct UPRN#2056InertiaUK wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 27 minutes and 1 second. 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)
📝 WalkthroughWalkthroughparse_data now requires postcode (optionally paon/uprn), performs an ASP.NET tokenized postcode search to find an address PID, then fetches the calendar for that PID and defensively parses upcoming collection dates within eight weeks, skipping cancelled or malformed entries. ChangesAddress search and calendar parsing refactor
Sequence DiagramsequenceDiagram
participant Client
participant NewarkSite as Newark & Sherwood Site
participant AddressParser
Client->>NewarkSite: GET root (extract __VIEWSTATE, __VIEWSTATEGENERATOR, __EVENTVALIDATION)
NewarkSite-->>Client: HTML with tokens
Client->>NewarkSite: POST search with postcode and tokens
NewarkSite-->>Client: HTML with address list (collection.aspx?pid=...)
AddressParser->>AddressParser: Extract PIDs and link list
AddressParser->>AddressParser: Select best PID (exact UPRN -> PAON match -> first)
AddressParser-->>Client: selected PID
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 #2056 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 9 9
Lines 1141 1141
=======================================
Hits 989 989
Misses 152 152 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py (1)
29-29: 💤 Low valueConsider documenting
verify=Falseor suppressing the urllib3 warning.Disabling SSL verification (
verify=False) is used throughout this file. While this may be necessary due to council site certificate issues, it could emitInsecureRequestWarninglogs. Consider either:
- Adding a comment explaining why verification is disabled, or
- Suppressing the warning locally if it clutters logs
import urllib3 urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)🤖 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/NewarkAndSherwoodDC.py` at line 29, The request call s.get(..., verify=False) in NewArkAndSherwoodDC.py disables SSL verification and can trigger urllib3 InsecureRequestWarning; either add a brief inline comment by the verify=False usage explaining why verification is disabled (e.g., council site cert issues) or, to suppress noisy warnings globally for this module, import urllib3 and call urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) near the top of the file (ensure import urllib3 is added) so the warning is not emitted while keeping the verify=False behavior for the s.get call.
🤖 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/NewarkAndSherwoodDC.py`:
- Around line 33-35: The three statements that extract hidden form tokens
(viewstate, viewstate_gen, event_val) use soup1.find(...)[ "value" ] and will
produce a cryptic TypeError if an input is missing; update the code around the
soup1.find calls (the extraction of __VIEWSTATE, __VIEWSTATEGENERATOR,
__EVENTVALIDATION) to check that each find returned a non-None element and that
the "value" attribute exists, and if any are missing raise a clear, explicit
exception (e.g., ValueError or RuntimeError) with a descriptive message
indicating which token is missing so failures are obvious and debuggable.
---
Nitpick comments:
In `@uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py`:
- Line 29: The request call s.get(..., verify=False) in NewArkAndSherwoodDC.py
disables SSL verification and can trigger urllib3 InsecureRequestWarning; either
add a brief inline comment by the verify=False usage explaining why verification
is disabled (e.g., council site cert issues) or, to suppress noisy warnings
globally for this module, import urllib3 and call
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) near the top
of the file (ensure import urllib3 is added) so the warning is not emitted while
keeping the verify=False behavior for the s.get call.
🪄 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: 9a57e4ee-babe-4218-ba96-59851d3eac63
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py
InertiaUK
force-pushed
the
fix-newark-postcode-lookup
branch
from
5e575ab to
f890a09
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py (1)
29-29: 💤 Low valueSSL certificate verification is disabled.
verify=Falsedisables certificate validation on lines 29, 51, and 100. While this may be necessary due to certificate issues on the council's server, it exposes the connection to potential MITM attacks.If certificate issues are temporary or fixable, consider enabling verification. Otherwise, document why it's disabled.
🤖 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/NewarkAndSherwoodDC.py` at line 29, The requests in NewarkAndSherwoodDC.py call s.get(...) with verify=False (e.g., the call setting r1 = s.get(f"{base}/", verify=False)), which disables SSL certificate validation; change these calls to enable verification by removing verify=False or by wiring a configurable flag (e.g., ssl_verify or REQUESTS_VERIFY) into the session usage so verification can be toggled safely, and if you must keep verification disabled add a clear comment and rationale near the session/request creation explaining why and how long it should remain disabled.
🤖 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/NewarkAndSherwoodDC.py`:
- Around line 75-96: The selection code currently falls back to address_links[0]
even when a user supplied user_uprn or user_paon but no match was found; change
the logic in the selection block (variables: selected, address_links, user_uprn,
user_paon) so that if user_uprn or user_paon was provided and no matching addr
is found you raise a clear exception (e.g., ValueError or a custom
AddressNotFound/LookupError) with a message that includes the supplied
uprn/paon, instead of silently assigning address_links[0]; only use the first
result as a silent fallback when neither user_uprn nor user_paon was provided.
---
Nitpick comments:
In `@uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py`:
- Line 29: The requests in NewarkAndSherwoodDC.py call s.get(...) with
verify=False (e.g., the call setting r1 = s.get(f"{base}/", verify=False)),
which disables SSL certificate validation; change these calls to enable
verification by removing verify=False or by wiring a configurable flag (e.g.,
ssl_verify or REQUESTS_VERIFY) into the session usage so verification can be
toggled safely, and if you must keep verification disabled add a clear comment
and rationale near the session/request creation explaining why and how long it
should remain disabled.
🪄 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: 80ed9fab-7ab0-464f-9caa-27f101800b63
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py
1 participant
Summary
__EVENTTARGETPOST to resolve correctpidfrom council's own address listTest
Summary by CodeRabbit
New Features
Bug Fixes