Conversation
Closes #84 Generated with Codex CLI
Summary of ChangesHello @BadgerHobbs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new data collector specifically designed for Mansfield Council, integrating its bin collection schedule into the existing system. The implementation involves a multi-step client-side request process to first obtain an authentication token and then retrieve address options, followed by an API call to fetch detailed bin collection dates. This enhancement expands the coverage of supported councils, allowing users in Mansfield to access their bin collection information. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new collector for Mansfield council. The implementation is well-structured and follows the multi-step request pattern required for this collector. The integration tests are also included and look correct.
I've identified a critical security vulnerability related to a hardcoded API key. While the style guide seems to permit this, it's a significant risk and should be addressed by using a secure secret management solution.
Additionally, I've left a few minor comments to align the code more closely with the repository's style guide regarding member ordering, object initialization patterns, and code comments.
Overall, great work on the new collector. Addressing the security issue is the highest priority.
| /// <inheritdoc/> | ||
| public override string GovUkId => "mansfield"; | ||
|
|
||
| private const string FormUrl = "https://www.mansfield.gov.uk/xfp/form/1339"; |
There was a problem hiding this comment.
private fields should have summary docstrings. Also naming convention is wrong, run dotnet format info severity.
| private const string FormUrl = "https://www.mansfield.gov.uk/xfp/form/1339"; | ||
| private const string FormPage = "2615"; | ||
| private const string FormId = "1339"; | ||
| private const string ApiUrl = "https://portal.mansfield.gov.uk/mdcwhitespacewebservice/WhiteSpaceWS.asmx/GetCollectionByUPRNAndDatePlus"; |
There was a problem hiding this comment.
remove constants with just on reference.
| {"locale", "en_GB"}, | ||
| {"injectedParams", $"{{\"formID\":\"{FormId}\"}}"}, | ||
| {"q3fc8e993e4e89b244317c1f13b6d65c0b0ef1ad2_0_0", formattedPostcode}, | ||
| {"callback", "{\"action\":\"ic\",\"element\":\"q3fc8e993e4e89b244317c1f13b6d65c0b0ef1ad2\",\"data\":0,\"tableRow\":-1}"}, |
There was a problem hiding this comment.
the value being escaped json string isn't great for readability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new collector for Mansfield council. While the implementation is well-structured, it has critical security concerns regarding a hardcoded API key and potential URL injection in the GetBinDays method that need immediate attention. Additionally, there are several style and convention improvements needed, including correct postcode handling, member ordering, and avoiding single-use constants, to align with the BinDays-API Collector Style Guide.
|
@claude Resolve the PR comments, making sure to validate/test the changes and check against style guide. |
|
Claude finished @BadgerHobbs's task in 3m 58s —— View job Resolving PR Review Comments ✓I've successfully addressed all the PR review comments and validated the changes against the style guide. Changes MadeStyle Guide Compliance:
Note on Items NOT Changed:
Validation✅ Formatting: Ran Files Modified
--- | Branch |
- Remove FormatPostcode call (postcode is already formatted) - Reorder class members (_binTypes before const fields) - Inline single-use constants (FormPage, FormId, ApiUrl) - Add trailing commas to multi-line initializers - Use separate variable declarations instead of inline returns - Add standard comments before foreach loops - Use TryGetValue for header access instead of direct indexer - Add XML documentation comments for private fields and regex methods Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
BadgerHobbs
left a comment
BadgerHobbs
left a comment
There was a problem hiding this comment.
@claude Resolve my new pr comments
|
Claude finished @BadgerHobbs's task —— View job Resolving PR Comments ✓I've successfully addressed all the PR review comments. Changes Made:
Validation:
Files Modified:
--- | Branch |
- Add XML summary docstring for _formUrl const field - Remove single-use _apiKey const and inline the value - TryGetValue usage confirmed correct per style guide Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
Changed from TryGetValue pattern to direct indexer access with ! operator since the set-cookie header is required for this collector. Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
2 participants
Summary
This PR adds a new bin collection data collector for Mansfield.
ICollectorinterfaceCloses #84
Test Summary
Generated automatically by Codex CLI