Fix location hierarchy fetch with _syncLocations#129
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with location hierarchy fetching when using the _syncLocations parameter by ensuring proper pagination handling and correct admin level filtering. The key change introduces a new method to collect all locations from paginated FHIR bundle responses.
Key changes:
- Added
collectAllLocations()method to properly handle paginated bundle results by following next links - Fixed
generateAdminLevels()to handle blank strings (not just null) as default values - Updated calls to use actual admin level parameters instead of hardcoded defaults for pre-fetch operations
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| LocationHierarchyEndpointHelper.java | Added pagination handling method and fixed admin level parameter usage in pre-fetch operations |
| BaseFhirEndpointHelper.java | Enhanced generateAdminLevels() to treat blank strings as null using StringUtils.isNotBlank() |
| LocationHierarchyEndpointHelperTest.java | Added tests for blank string handling and pagination, updated mock verifications to handle multiple calls |
| SyncAccessDecision.java | Added debug logging and reformatted javadoc comment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (logger.isInfoEnabled()) { | ||
| String proxyBase = System.getenv(Constants.PROXY_TO_ENV); | ||
| logger.info( | ||
| "Proxying {} request to PROXY_TO base '{}' with path '{}'", | ||
| method, | ||
| proxyBase, | ||
| requestPath); | ||
| } |
There was a problem hiding this comment.
The environment variable is retrieved on every invocation of this static method. Consider caching System.getenv(Constants.PROXY_TO_ENV) to avoid repeated lookups, or move the retrieval outside the info-enabled check if this method is frequently called.
|
@dubdabasoduba I've opened a new pull request, #130, to work on those changes. Once the pull request is ready, I'll request review from you. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
============================================
+ Coverage 64.35% 67.04% +2.69%
- Complexity 296 310 +14
============================================
Files 19 19
Lines 2087 2109 +22
Branches 272 280 +8
============================================
+ Hits 1343 1414 +71
+ Misses 612 561 -51
- Partials 132 134 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…t calls Co-authored-by: dubdabasoduba <4817531+dubdabasoduba@users.noreply.github.com>
Eliminate redundant getFhirClientForR4() calls in location pagination
…-extension into fix_location_fetching
IMPORTANT: Where possible all PRs must be linked to a Github issue
Resolves [link to issue]
Engineer Checklist
bug fixes
option(s) on the
README.mdmvn spotless:checkto check my code follows the project'sstyle guide
mvn clean test jacoco:reportto confirm the coverage reportwas generated at
plugins/target/site/jacoco/index.htmlmvn clean packageright before creating this pull request.