Add collector for Cheltenham Borough Council#135
Add collector for Cheltenham Borough Council#135moley-bot[bot] wants to merge 2 commits intomainfrom
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new collector for Cheltenham Borough Council. While comprehensive, it has several security and reliability issues. The code is vulnerable to denial-of-service (DoS) attacks due to improper handling of malformed external responses in ParseJsonp and GetWeekStartDate. Missing URL encoding for client-side requests also poses a risk to functionality (e.g., with postcodes containing spaces) and security (potential parameter injection). Furthermore, the implementation is complex, particularly in the GetBinDays method, which exceeds recommended length, and exhibits code duplication and style guide deviations. Refactoring suggestions include creating helper methods for session/task logic, inlining constants, and improving collection/loop style.
BinDays.Api.Collectors/Collectors/Councils/CheltenhamBoroughCouncil.cs
Outdated
Show resolved
Hide resolved
BinDays.Api.Collectors/Collectors/Councils/CheltenhamBoroughCouncil.cs
Outdated
Show resolved
Hide resolved
BinDays.Api.Collectors/Collectors/Councils/CheltenhamBoroughCouncil.cs
Outdated
Show resolved
Hide resolved
BinDays.Api.Collectors/Collectors/Councils/CheltenhamBoroughCouncil.cs
Outdated
Show resolved
Hide resolved
BinDays.Api.Collectors/Collectors/Councils/CheltenhamBoroughCouncil.cs
Outdated
Show resolved
Hide resolved
BinDays.Api.Collectors/Collectors/Councils/CheltenhamBoroughCouncil.cs
Outdated
Show resolved
Hide resolved
BinDays.Api.Collectors/Collectors/Councils/CheltenhamBoroughCouncil.cs
Outdated
Show resolved
Hide resolved
|
@claude Rebase onto main and update the integration test to the new format. |
|
Claude finished @BadgerHobbs's task in 2m 13s —— View job Task completedTodo List
SummarySuccessfully rebased the branch onto Changes made:
The branch is now clean with a single commit on top of main, ready for review. --- | |
- Implements ICollector interface - Adds integration test using new format - Successfully tested with example postcode from issue Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
46dd180 to
9091b1c
Compare
|
@claude Please fix all unresolved PR comments, making sure to test after each change made. Please include the test output in your final comment update. |
|
Claude finished @BadgerHobbs's task in 4m 55s —— View job Task completedTodo List
SummaryAll PR review comments have been addressed: BadgerHobbs's comments:
Gemini's comments (security/medium priority):
Test Output| |
- Remove bin type/color suffixes from bin names (Green Box, Blue Bag, Green Caddy) - Add trailing comma on last bin type - Inline single-use _calendarUrl constant - Replace string Replace calls with GeneratedRegex for HTML bold tag stripping - Add GeneratedRegex (make class partial) - Fix raw string literal indentation in job JSON bodies - Add ParseJsonp bounds validation to prevent InvalidOperationException on malformed JSONP - Add GetWeekStartDate colon validation before Split to prevent IndexOutOfRangeException - Extract CreateSessionRequest helper method (used in both GetAddresses and GetBinDays) - Inline ParseDayOfWeek single-use helper method into BuildCollectionDates - Refactor duplicate foreach loops into single combined loop over service tuples Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
Summary
This PR adds a new bin collection data collector for Cheltenham Borough Council.
ICollectorinterfaceCloses #131
Test Summary
Generated automatically by Moley-Bot using Codex CLI