-
-
Notifications
You must be signed in to change notification settings - Fork 4
Add collector for London Borough of Waltham Forest #129
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?
Add collector for London Borough of Waltham Forest #129
Conversation
Closes #106 Generated with Codex CLI by Moley-Bot
Formatted by Moley-Bot
| }, | ||
| ]; | ||
|
|
||
| private const string _achieveFormsUrl = "https://portal.walthamforest.gov.uk/AchieveForms/?mode=fill&consentMessage=yes&form_uri=sandbox-publish://AF-Process-d62ccdd2-3de9-48eb-a229-8e20cbdd6393/AF-Stage-8bf39bf9-5391-4c24-857f-0dc2025c67f4/definition.json&process=1&process_uri=sandbox-processes://AF-Process-d62ccdd2-3de9-48eb-a229-8e20cbdd6393&process_id=AF-Process-d62ccdd2-3de9-48eb-a229-8e20cbdd6393"; |
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.
Remove constants with only one reference.
| ]; | ||
|
|
||
| private const string _achieveFormsUrl = "https://portal.walthamforest.gov.uk/AchieveForms/?mode=fill&consentMessage=yes&form_uri=sandbox-publish://AF-Process-d62ccdd2-3de9-48eb-a229-8e20cbdd6393/AF-Stage-8bf39bf9-5391-4c24-857f-0dc2025c67f4/definition.json&process=1&process_uri=sandbox-processes://AF-Process-d62ccdd2-3de9-48eb-a229-8e20cbdd6393&process_id=AF-Process-d62ccdd2-3de9-48eb-a229-8e20cbdd6393"; | ||
| private const string _apibrokerBaseUrl = "https://portal.walthamforest.gov.uk/apibroker/"; |
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.
Add missing docstrings for consts.
| // Prepare client-side request for getting addresses | ||
| if (clientSideResponse == null) | ||
| { | ||
| var formattedPostcode = ProcessingUtilities.FormatPostcode(postcode); |
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.
Postcode does not need to be formatted.
| ); | ||
| var formattedPostcode = clientSideResponse.Options.Metadata["postcode"]; | ||
|
|
||
| var requestBody = $$""" |
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.
All these form fields may not be required.
| }, | ||
| Options = new ClientSideOptions | ||
| { | ||
| Metadata = |
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.
Don't include psotcode in metadata.
| // Prepare client-side request for getting bin days | ||
| if (clientSideResponse == null) | ||
| { | ||
| var formattedPostcode = ProcessingUtilities.FormatPostcode(address.Postcode!); |
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.
Don't need to format postcode.
| { | ||
| Metadata = | ||
| { | ||
| { "postcode", formattedPostcode }, |
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.
Remove postcode and uprn from metadata.
| var addressDisplay = matchedAddress!.Value.GetProperty("display").GetString()!; | ||
| var ward = matchedAddress.Value.GetProperty("overview_ward").GetString()!; | ||
|
|
||
| var requestBody = $$""" |
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.
May be able to remove dields here from form.
| var formattedPostcode = clientSideResponse.Options.Metadata["postcode"]; | ||
| var ward = clientSideResponse.Options.Metadata["ward"]; | ||
|
|
||
| var requestBody = $$""" |
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.
May be able to remove properties from form.
| { | ||
| Metadata = | ||
| { | ||
| { "address", addressDisplay }, |
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.
Do we need this int the metadata?
|
/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 adds a new collector for the London Borough of Waltham Forest. The implementation is quite complex due to the multi-step API interaction, and you've done a good job navigating it. The code correctly follows the state-passing and step-by-step request patterns outlined in the style guide.
My main feedback focuses on improving maintainability by adhering more closely to the repository's style guide. Specifically, I've suggested:
- Simplifying the very large and verbose JSON request bodies by removing unnecessary fields.
- Refactoring duplicated logic into helper methods.
- Removing unused constants and metadata.
These changes will make the collector easier to read and maintain in the future. The integration test is well-written and follows the project's template. Overall, great work on a challenging collector!
| var requestBody = $$""" | ||
| { | ||
| "stopOnFailure": true, | ||
| "stage_id": "{{_stageId}}", | ||
| "stage_name": "Enquiry", | ||
| "formId": "{{_addressFormId}}", | ||
| "formValues": { | ||
| "Section 1": { | ||
| "blankLabel": { | ||
| "name": "blankLabel", | ||
| "type": "text", | ||
| "id": "AF-Field-9ab181c8-c72d-43e8-91ea-17f9b632617f", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "blankLabel", | ||
| "value": "", | ||
| "path": "root/addressLookup/blankLabel", | ||
| "valid": "" | ||
| }, | ||
| "postcode_search": { | ||
| "name": "postcode_search", | ||
| "type": "text", | ||
| "id": "AF-Field-648c89c8-ba21-49e0-b877-c23736d00e27", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "Enter your postcode", | ||
| "value": "{{formattedPostcode}}", | ||
| "path": "root/addressLookup/postcode_search", | ||
| "valid": true | ||
| }, | ||
| "postcodeFound": { | ||
| "name": "postcodeFound", | ||
| "type": "text", | ||
| "id": "AF-Field-a46bf8a7-377a-4179-9234-869f03170b10", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "postcodeFound", | ||
| "value": "", | ||
| "path": "root/addressLookup/postcodeFound", | ||
| "valid": "" | ||
| }, | ||
| "uprnConfirm": { | ||
| "name": "uprnConfirm", | ||
| "type": "text", | ||
| "id": "AF-Field-95ab2ef2-a616-4dbc-a0a7-86f5f88b2c17", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "uprnConfirm", | ||
| "value": "", | ||
| "path": "root/addressLookup/uprnConfirm", | ||
| "valid": "" | ||
| }, | ||
| "wardName": { | ||
| "name": "wardName", | ||
| "type": "text", | ||
| "id": "AF-Field-0d01d6b6-eb90-4f20-8bd0-3cd6c3afb7f4", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "wardName", | ||
| "value": "", | ||
| "path": "root/addressLookup/wardName", | ||
| "valid": "" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """; |
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.
This request body is very large and includes many fields with empty string values or static default values. The style guide emphasizes creating minimal request payloads by removing unnecessary fields.
To improve readability and align with the guide, please simplify this payload to only include fields that are strictly required by the API. This same feedback applies to the other large, verbose request bodies in this collector (e.g., lines 294, 420, 647).
var requestBody = $$ """
{
"formId": "{{_addressFormId}}",
"formValues": {
"Section 1": {
"postcode_search": {
"value": "{{formattedPostcode}}"
}
}
}
}
""";References
- Request bodies should be minimal. Do not include fields with empty, null, default, or false values unless the API strictly requires them. Always question if a field is truly necessary. (link)
| else if (clientSideResponse.RequestId == 1) | ||
| { | ||
| var sid = SessionIdRegex().Match(clientSideResponse.Content).Groups[1].Value; | ||
| var requestCookies = ProcessingUtilities.ParseSetCookieHeaderForRequestCookie( | ||
| clientSideResponse.Headers["set-cookie"] | ||
| ); | ||
| var formattedPostcode = clientSideResponse.Options.Metadata["postcode"]; | ||
|
|
||
| var requestBody = $$""" | ||
| { | ||
| "stopOnFailure": true, | ||
| "stage_id": "{{_stageId}}", | ||
| "stage_name": "Enquiry", | ||
| "formId": "{{_addressFormId}}", | ||
| "formValues": { | ||
| "Section 1": { | ||
| "blankLabel": { | ||
| "name": "blankLabel", | ||
| "type": "text", | ||
| "id": "AF-Field-9ab181c8-c72d-43e8-91ea-17f9b632617f", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "blankLabel", | ||
| "value": "", | ||
| "path": "root/addressLookup/blankLabel", | ||
| "valid": "" | ||
| }, | ||
| "postcode_search": { | ||
| "name": "postcode_search", | ||
| "type": "text", | ||
| "id": "AF-Field-648c89c8-ba21-49e0-b877-c23736d00e27", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "Enter your postcode", | ||
| "value": "{{formattedPostcode}}", | ||
| "path": "root/addressLookup/postcode_search", | ||
| "valid": true | ||
| }, | ||
| "postcodeFound": { | ||
| "name": "postcodeFound", | ||
| "type": "text", | ||
| "id": "AF-Field-a46bf8a7-377a-4179-9234-869f03170b10", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "postcodeFound", | ||
| "value": "", | ||
| "path": "root/addressLookup/postcodeFound", | ||
| "valid": "" | ||
| }, | ||
| "uprnConfirm": { | ||
| "name": "uprnConfirm", | ||
| "type": "text", | ||
| "id": "AF-Field-95ab2ef2-a616-4dbc-a0a7-86f5f88b2c17", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "uprnConfirm", | ||
| "value": "", | ||
| "path": "root/addressLookup/uprnConfirm", | ||
| "valid": "" | ||
| }, | ||
| "wardName": { | ||
| "name": "wardName", | ||
| "type": "text", | ||
| "id": "AF-Field-0d01d6b6-eb90-4f20-8bd0-3cd6c3afb7f4", | ||
| "value_changed": true, | ||
| "section_id": "AF-Section-cc45faa3-b04a-4815-81c4-261ff6cd94f2", | ||
| "label": "wardName", | ||
| "value": "", | ||
| "path": "root/addressLookup/wardName", | ||
| "valid": "" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """; | ||
|
|
||
| var clientSideRequest = new ClientSideRequest | ||
| { | ||
| RequestId = 2, | ||
| Url = $"{_apibrokerBaseUrl}?api=RunLookup&id={_addressLookupId}&repeat_against=&noRetry=false&getOnlyTokens=undefined&log_id=&app_name=AF-Renderer::Self&sid={sid}", | ||
| Method = "POST", | ||
| Headers = new() | ||
| { | ||
| { "content-type", "application/json" }, | ||
| { "cookie", requestCookies }, | ||
| { "x-requested-with", "XMLHttpRequest" }, | ||
| }, | ||
| Body = requestBody, | ||
| Options = new ClientSideOptions | ||
| { | ||
| Metadata = | ||
| { | ||
| { "cookie", requestCookies }, | ||
| { "sid", sid }, | ||
| { "postcode", formattedPostcode }, | ||
| { "uprn", address.Uid! }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| var getBinDaysResponse = new GetBinDaysResponse | ||
| { | ||
| NextClientSideRequest = clientSideRequest, | ||
| }; | ||
|
|
||
| return getBinDaysResponse; |
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.
This entire block for handling RequestId == 1 is nearly identical to the corresponding block in the GetAddresses method (lines 106-210). This creates significant code duplication.
To improve maintainability, consider extracting this shared logic for preparing the address lookup request into a private helper method. This helper could then be called from both GetAddresses and GetBinDays.
References
- Helper methods should be created to reduce code duplication, especially for logic that is used in 2-3 or more places. (link)
| private const string _setAddressLookupId = "5e42e28b44d9e"; | ||
| private const string _collectionsLookupId = "5e208cda0d0a0"; |
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.
These constants, _setAddressLookupId and _collectionsLookupId, are each used only once in the file. According to the repository style guide, constants should be inlined if they are not used at least twice.
To improve adherence to the style guide and reduce unnecessary field declarations, please inline these values at their usage sites (lines 607 and 834 respectively) and remove these constant definitions.
References
- Constants should only be created if they are used two or more times. If a value is only used once, it should be inlined directly at the site of use. (link)
| Options = new ClientSideOptions | ||
| { | ||
| Metadata = | ||
| { | ||
| { "cookie", requestCookies }, | ||
| { "sid", sid }, | ||
| { "postcode", formattedPostcode }, | ||
| }, | ||
| }, |
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 metadata keys cookie, sid, and postcode are stored here but are not used in the subsequent step of the GetAddresses flow (the response is returned at RequestId == 2). The style guide advises against storing unnecessary data in metadata.
References
- Only store data in metadata that is necessary for subsequent request steps. Avoid adding keys that are not used later in the flow. (link)
| JsonElement? matchedAddress = null; | ||
| var uprn = clientSideResponse.Options.Metadata["uprn"]; | ||
| foreach (var property in rowsData.EnumerateObject()) | ||
| { | ||
| if (property.Value.GetProperty("overview_uprn").GetString()! == uprn) | ||
| { | ||
| matchedAddress = property.Value; | ||
| break; | ||
| } | ||
| } |
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.
This foreach loop to find the matching address can be written more concisely and idiomatically using LINQ's FirstOrDefault method.
var uprn = clientSideResponse.Options.Metadata["uprn"];
var matchedAddress = rowsData.EnumerateObject()
.Select(p => p.Value)
.FirstOrDefault(v => v.GetProperty("overview_uprn").GetString()! == uprn);| var binDayEntries = new List<JsonElement>(); | ||
| if (rowsData.ValueKind == JsonValueKind.Object) | ||
| { | ||
| foreach (var property in rowsData.EnumerateObject()) | ||
| { | ||
| binDayEntries.Add(property.Value); | ||
| } | ||
| } | ||
| else if (rowsData.ValueKind == JsonValueKind.Array) | ||
| { | ||
| binDayEntries.AddRange(rowsData.EnumerateArray()); | ||
| } |
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.
This logic to handle rowsData being either a JSON object or an array can be simplified. Using a ternary operator with LINQ can make this more concise and improve readability.
var binDayEntries = rowsData.ValueKind == JsonValueKind.Object
? rowsData.EnumerateObject().Select(p => p.Value)
: rowsData.EnumerateArray();|
@claude Please resolve all outstanding PR comments, be thourough, and test the changes you make and check against the style guide. |
|
Claude finished @BadgerHobbs's task in 6m 48s —— View job Resolved All PR Review Comments ✓Successfully addressed all outstanding review comments from both BadgerHobbs and gemini-code-assist reviewers. Completed Tasks
Changes MadeCode Quality Improvements:
Request Payload Optimization:
Metadata Cleanup:
Code Modernization:
Testing:
SummaryThe collector has been thoroughly refactored to align with the repository's style guide and address all reviewer feedback. The code is now:
|
…comments - Remove single-use constants (_setAddressLookupId, _collectionsLookupId) and inline them - Add XML docstrings for all const fields - Remove duplicate postcode formatting (only format once per method) - Simplify large request bodies by removing unnecessary fields - Remove unnecessary metadata (postcode/uprn from final requests) - Extract duplicated address lookup logic into BuildAddressLookupPayload helper - Extract duplicated main form logic into BuildMainFormPayload helper - Use LINQ FirstOrDefault for address matching instead of foreach loop - Simplify rowsData handling with ternary operator - All integration tests pass successfully Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
Formatted by Moley-Bot
Summary
This PR adds a new bin collection data collector for London Borough of Waltham Forest.
ICollectorinterfaceCloses #106
Test Summary
Generated automatically by Moley-Bot using Codex CLI