-
-
Notifications
You must be signed in to change notification settings - Fork 4
Add collector for North Devon Council #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
79e0514 to
af5b993
Compare
Closes #22 Generated with Codex CLI
af5b993 to
ba4f3c6
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new collector for North Devon Council. The implementation is quite complex, involving a multi-step process to retrieve data, which has been handled using the established if/else if pattern. The code is well-structured, and the inclusion of integration tests is excellent.
My review focuses on several areas for improvement:
- Adherence to the repository's C# style guide, particularly regarding object initializers and using framework utilities.
- Removal of unused code (usings and regex methods).
- Opportunities to reduce significant code duplication by extracting common logic into helper methods.
- A critical issue with hardcoded bin matching logic that should be refactored to use the configured bin types for better maintainability.
These changes will improve the code's quality, readability, and robustness.
| if (workPack.StartsWith("Waste-Black", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| binsForDate.Add(generalWasteBin); | ||
| } | ||
| else if (workPack.StartsWith("Waste-Recycling", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| binsForDate.Add(recyclingBin); | ||
| } | ||
| else | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Replaced hardcoded bin matching logic with ProcessingUtilities.GetMatchingBins(_binTypes, workPack) in both step 8 and step 9 (retry logic). Also updated the bin type keys to match the actual data format (Waste-Black, Waste-Recycling).
| private readonly IReadOnlyCollection<Bin> _binTypes = | ||
| [ | ||
| new() | ||
| { | ||
| Name = "General Waste", | ||
| Colour = BinColour.Black, | ||
| Type = BinType.Bin, | ||
| Keys = [ "Black Bin", "BlackBin" ], | ||
| }, | ||
| new() | ||
| { | ||
| Name = "Recycling", | ||
| Colour = BinColour.Green, | ||
| Type = BinType.Container, | ||
| Keys = [ "Recycling" ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Updated bin type keys to 'Waste-Black' and 'Waste-Recycling' to match the actual data format. Trailing commas were already present from the formatter.
| private static partial Regex MonthSectionRegex(); | ||
|
|
||
| /// <inheritdoc/> | ||
| public GetAddressesResponse GetAddresses(string postcode, ClientSideResponse? clientSideResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Extracted the first three request steps (session initialization and authentication) into a shared HandleSessionInitialization helper method used by both GetAddresses and GetBinDays. This eliminates the duplication.
| return getBinDaysResponse; | ||
| } | ||
| // Process bin days from response | ||
| else if (clientSideResponse.RequestId == 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Extracted the duplicated parsing logic into a ParseBinDaysFromRowsData helper method that is now used by both step 8 and step 9 (retry logic).
| using System.Linq; | ||
| using System.Text.Json; | ||
| using System.Text.RegularExpressions; | ||
| using System.Web; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Removed the unused using System.Web; statement.
| /// <summary> | ||
| /// Regex for matching individual collection entries. | ||
| /// </summary> | ||
| [GeneratedRegex(@"<li class=\""(?<binKey>[^\""]+)\""[^>]*>\s*<span class=\""wasteName\"">(?<dayName>[^<]+)</span>\s*<span class=\""wasteDay\"">(?<day>\d{2})</span>\s*<span class=\""wasteType\"">(?<binLabel>[^<]+)</span>", RegexOptions.Singleline)] | ||
| private static partial Regex CollectionItemRegex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Removed the unused CollectionItemRegex method.
| /// <summary> | ||
| /// Regex to match month sections and their collection items. | ||
| /// </summary> | ||
| [GeneratedRegex(@"<h4>(?<monthYear>[A-Za-z]+\s+\d{4})</h4>(?<items>.*?)(?=<h4>[A-Za-z]+\s+\d{4}</h4>|<h2>|$)", RegexOptions.Singleline)] | ||
| private static partial Regex MonthSectionRegex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Removed the unused MonthSectionRegex method.
| // Prepare client-side request for getting location data | ||
| else if (clientSideResponse.RequestId == 2) | ||
| { | ||
| var previousCookies = clientSideResponse.Options.Metadata.GetValueOrDefault("cookies", string.Empty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Added validation in the \ helper method to check for the presence of the set-cookie header and throw an \ if it's missing.
| { | ||
| var previousCookies = clientSideResponse.Options.Metadata.GetValueOrDefault("cookies", string.Empty); | ||
| var newCookies = ProcessingUtilities.ParseSetCookieHeaderForRequestCookie(clientSideResponse.Headers["set-cookie"]); | ||
| var cookies = CombineCookies(previousCookies, newCookies); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept as local method. This is the only collector using CombineCookies - I checked the codebase and no other collectors have similar logic. If it becomes common in future collectors, we can extract it to ProcessingUtilities then.
| var sid = clientSideResponse.Options.Metadata.GetValueOrDefault("sid", string.Empty); | ||
| var formattedPostcode = clientSideResponse.Options.Metadata.GetValueOrDefault("postcode", string.Empty); | ||
|
|
||
| var requestBody = JsonSerializer.Serialize(new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment explaining that many fields are empty but required by the API. Removing them could cause the API to reject the request.
| return getBinDaysResponse; | ||
| } | ||
| // Prepare client-side request for getting address details | ||
| else if (clientSideResponse.RequestId == 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the multi-step session setup is required for each GetBinDays call. The API needs fresh session cookies and SID for the bin collection lookup process. Added a comment explaining this requirement.
| return getBinDaysResponse; | ||
| } | ||
| // Process bin days from retry response | ||
| else if (clientSideResponse.RequestId == 9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment clarifying this. The retry response (step 9) is used when the initial request (step 8) returns no bin collection data. It retries the same request to handle cases where the API may not return data on the first attempt.
| throw new InvalidOperationException("Invalid client-side request."); | ||
| } | ||
|
|
||
| private static string BuildCalendarRequestBody( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BuildCalendarRequestBody method centralizes the complex request body structure used for calendar lookups, and is called by CreateCalendarLookupRequest. While CreateCalendarLookupRequest is only used twice, it provides clear separation of concerns. Added documentation comments to both methods to clarify their purpose.
| noRetry); | ||
| } | ||
|
|
||
| private static ClientSideRequest CreateRunLookupRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The \ method encapsulates the common pattern for API broker requests with timestamp and retry parameters. While it's only called from one place, it provides clear abstraction and makes the code more maintainable. Added documentation comment to clarify its purpose.
- Remove unused using System.Web statement - Remove unused MonthSectionRegex and CollectionItemRegex methods - Fix bin type keys to match actual data (Waste-Black, Waste-Recycling) - Replace hardcoded bin matching with ProcessingUtilities.GetMatchingBins - Extract duplicated bin parsing logic into ParseBinDaysFromRowsData helper - Extract session initialization (steps 1-2) into HandleSessionInitialization helper - Add cookie validation in session initialization - Add comments explaining retry logic, empty request fields, and API requirements - Add documentation comments to helper methods All integration tests passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Formatted by Moley-Bot
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new collector for North Devon Council, including implementation of the ICollector interface and accompanying integration tests. The collector retrieves bin collection data from the council's website. The code adheres to the repository's style guide, with some minor suggestions for improvement.
| private readonly IReadOnlyCollection<Bin> _binTypes = | ||
| [ | ||
| new() | ||
| { | ||
| Name = "General Waste", | ||
| Colour = BinColour.Black, | ||
| Type = BinType.Bin, | ||
| Keys = [ "Waste-Black" ], | ||
| }, | ||
| new() | ||
| { | ||
| Name = "Recycling", | ||
| Colour = BinColour.Green, | ||
| Type = BinType.Container, | ||
| Keys = [ "Waste-Recycling" ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using descriptive bin names instead of generic ones. For example, instead of "Recycling", use "Paper, Glass & Cardboard Recycling". Also, ensure that the keys accurately reflect what is being matched from the data source. See Bin Type Configuration in the style guide.
References
- Bin names should describe what goes in the bin, not just the container. Only add keys that will actually be matched against the data source. (link)
| // Note: Many fields below are empty but required by the API | ||
| var requestBody = JsonSerializer.Serialize(new | ||
| { | ||
| stopOnFailure = true, | ||
| usePHPIntegrations = true, | ||
| stage_id = _stageId, | ||
| stage_name = "Stage 1", | ||
| formId = _formId, | ||
| formValues = new Dictionary<string, object?> | ||
| { | ||
| ["Your address"] = new | ||
| { | ||
| qsUPRN = new { value = string.Empty }, | ||
| postcode_search = new { value = formattedPostcode }, | ||
| chooseAddress = new { value = string.Empty }, | ||
| uprnfromlookup = new { value = string.Empty }, | ||
| UPRNMF = new { value = string.Empty }, | ||
| FULLADDR2 = new { value = string.Empty }, | ||
| }, | ||
| ["Calendar"] = new Dictionary<string, object?> | ||
| { | ||
| { "FULLADDR", new { value = string.Empty } }, | ||
| { "token", new { value = string.Empty } }, | ||
| { "uPRN", new { value = string.Empty } }, | ||
| { "calstartDate", new { value = string.Empty } }, | ||
| { "calendDate", new { value = string.Empty } }, | ||
| { "details", Array.Empty<object>() }, | ||
| { "text1", new { value = string.Empty } }, | ||
| { "Results", new { value = string.Empty } }, | ||
| { "UPRN", new { value = string.Empty } }, | ||
| { "Alerts", new { value = string.Empty } }, | ||
| { "liveToken", new { value = string.Empty } }, | ||
| { "Results2", new { value = string.Empty } }, | ||
| { "USRN", new { value = string.Empty } }, | ||
| { "streetEvents", Array.Empty<object>() }, | ||
| { "EventDescription", new { value = string.Empty } }, | ||
| { "EventDate", new { value = string.Empty } }, | ||
| { "EventsDisplay", new { value = string.Empty } }, | ||
| { "Comments", new { value = string.Empty } }, | ||
| { "OutText", new { value = string.Empty } }, | ||
| { "StartDate", new { value = string.Empty } }, | ||
| { "EndDate", new { value = string.Empty } }, | ||
| }, | ||
| ["Print version"] = new | ||
| { | ||
| OutText2 = new { value = string.Empty }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the style guide, request bodies should be minimal and contain only the necessary fields. Review the fields included in the request body to ensure that all unnecessary fields (empty strings, null values, default values, or false booleans) are removed. See Request Bodies & JSON Payloads in the style guide.
References
- Remove all unnecessary fields. Only include what the API actually requires. (link)
| Method = "POST", | ||
| Body = requestBody, | ||
| Headers = new() | ||
| { | ||
| { "content-type", "application/json" }, | ||
| { "cookie", cookies }, | ||
| { "x-requested-with", "XMLHttpRequest" }, | ||
| { "User-Agent", Constants.UserAgent }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style guide recommends using multi-line date parsing for improved readability. See Code Formatting & Style in the style guide.
var collectionDate = DateOnly.ParseExact(
match.Groups["date"].Value,
"dd/MM/yyyy",
CultureInfo.InvariantCulture,
DateTimeStyles.None
);References
- Multi-line format improves readability. (link)
| Options = new ClientSideOptions | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closing parenthesis should be on a separate line for multi-line method calls. See Code Formatting & Style in the style guide.
DateTimeStyles.None
);References
- Consistent formatting for multi-line method calls. (link)
Summary
This PR adds a new bin collection data collector for North Devon Council.
ICollectorinterfaceCloses #22
Test Summary
Generated automatically by Codex CLI