Conversation
Collaborator
j-corry
commented
Feb 5, 2026
- Look for string key in error hash from SES, not symbol, in content_…type_objects_controller.rb show
- Fix misnamed Solr field 'answeringDept_ses' in filter field names
- Refactor api_call.rb:
- Add some more comments
- Make non-public methods private as they should be
- Remove caching step for POST requests to Solr as we're unlikely to want this
- Fix encoding issues with Solr JSON responses
- Fix compression issues with Solr responses by allowing Net::HTTP to handle decompression as necessary, rather than managing this manually. Necessary as Solr will arbitrarily using gzip or not based on response size, which varies with query.
- Move testing boundary further down the stack so we're stubbing api_post_request instead of the method that evaluates & processes & what it returns, so that we can properly test said evaluation/processing; no non-trivial bespoke code remains untested.
- Refactor ses_lookup.rb & ses_query.rb in a similar fashion to the above, highlights include:
- Add better error handling when parsing SES responses: explicitly raise an error if we can't parse the XML for some reason (as well as fixing the same issue where a symbol key was being used to check for errors instead of a string key)
- Make sure we're using a standard base url for the API in the test environment
- DRY the api_get_request method from two into one, which handles caching based on a boolean arg (we want to cache the hierarchy response because it's slow, but ideally not the majority of SES calls at a request level, because we're using a dedicated caching system for the retrieved data further up the stack)
- Update / add to / simplify tests around the above API-related classes:
- A number of newly private methods have had their individual tests removed in favour of making sure they get hit during testing of the public methods, as it should be. Reduced test maintenance as a result.
- ApiCall is no longer tested directly as it's an abstract class. These tests were a stopgap, and it's now being tested thoroughly via its various subclasses instead.
- Update all the subclass specs to stub the response at the HTTP request level as mentioned previously, which means we're also ensuring the query is constructed as expected from the parameters too.
- Add totally new tests for a bunch of things that weren't properly covered.
- Add some fixtures (example XML error from SES API, SES JSON hierarchy response, example processed hierarchy data Ruby literal) which are used in some of the new tests
- Update gemfile:
- Remove 'temporarily required until Rails update with Ruby 3.4' bits (we're on 4.0.1 already)
- Add missing fiddle gem
…type_objects_controller.rb show - Fix misnamed Solr field 'answeringDept_ses' in filter field names - Refactor api_call.rb: -- Add some more comments -- Make non-public methods private as they should be -- Remove caching step for POST requests to Solr as we're unlikely to want this -- Fix encoding issues with Solr JSON responses -- Fix compression issues with Solr responses by allowing Net::HTTP to handle decompression as necessary, rather than managing this manually. Necessary as Solr will arbitrarily using gzip or not based on response size, which varies with query. -- Move testing boundary further down the stack so we're stubbing api_post_request instead of the method that evaluates & processes & what it returns, so that we can properly test said evaluation/processing; no non-trivial bespoke code remains untested. - Refactor ses_lookup.rb & ses_query.rb in a similar fashion to the above, highlights include: -- Add better error handling when parsing SES responses: explicitly raise an error if we can't parse the XML for some reason (as well as fixing the same issue where a symbol key was being used to check for errors instead of a string key) -- Make sure we're using a standard base url for the API in the test environment -- DRY the api_get_request method from two into one, which handles caching based on a boolean arg (we want to cache the hierarchy response because it's slow, but ideally not the majority of SES calls at a request level, because we're using a dedicated caching system for the retrieved data further up the stack) - Update / add to / simplify tests around the above API-related classes: -- A number of newly private methods have had their individual tests removed in favour of making sure they get hit during testing of the public methods, as it should be. Reduced test maintenance as a result. -- ApiCall is no longer tested directly as it's an abstract class. These tests were a stopgap, and it's now being tested thoroughly via its various subclasses instead. -- Update all the subclass specs to stub the response at the HTTP request level as mentioned previously, which means we're also ensuring the query is constructed as expected from the parameters too. -- Add totally new tests for a bunch of things that weren't properly covered. -- Add some fixtures (example XML error from SES API, SES JSON hierarchy response, example processed hierarchy data Ruby literal) which are used in some of the new tests - Update gemfile: -- Remove 'temporarily required until Rails update with Ruby 3.4' bits (we're on 4.0.1 already) -- Add missing fiddle gem
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.