-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for administrative levels search parameters on LocationHierarchy endpoint #66
base: main
Are you sure you want to change the base?
Conversation
…erarchy endpoint. fixes #64
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
============================================
+ Coverage 43.81% 44.89% +1.08%
- Complexity 106 123 +17
============================================
Files 15 15
Lines 1075 1156 +81
Branches 120 143 +23
============================================
+ Hits 471 519 +48
- Misses 545 570 +25
- Partials 59 67 +8 ☔ View full report in Codecov by Sentry. |
Tests
public static final String AUTHORIZATION = "Authorization"; | ||
public static final String KEYCLOAK_UUID = "keycloak-uuid"; | ||
public static final String IDENTIFIER = "_id"; | ||
public static final String MIN_ADMIN_LEVEL = "administrativeLevelMin"; | ||
public static final String MAX_ADMIN_LEVEL = "administrativeLevelMax"; | ||
public static final int DEFAULT_MAX_ADMIN_LEVEL = 10; |
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.
What is a reasonable default maximum admin level depth?
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.
I am not sure we can have a max
level because countries have different hierarchies. I know we can have the min
which is 0
but I don't think we can restrict the max
In case the max
is not defined we are supposed to fetch the whole hierarchy to the end of the node
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.
Seeing this is how the type element is represented:
"type": {
"coding": [
{
"system": "http://example.org/fhir/CodeSystem/location-type",
"code": "1",
"display": "Level 1"
}
],
querying directly with conditions like code is greater than 5
on a string representation of a code is not possible. FHIR does not support such numeric comparisons on string fields directly within the query. So, to handle this, I have implement post-query filtering in the application logic.
This commit addresses that 7d9ba8b
This means, api requests with adminstrativeLevelMax
set will be more optimized than those without.
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 makes sense. Can we review the performance with what we have as we think of how to address this?
plugins/src/main/java/org/smartregister/fhir/gateway/plugins/Constants.java
Outdated
Show resolved
Hide resolved
68a1567
to
b71884a
Compare
b71884a
to
7067ddf
Compare
...rc/test/java/org/smartregister/fhir/gateway/plugins/LocationHierarchyEndpointHelperTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/smartregister/fhir/gateway/plugins/LocationHierarchyEndpointHelperTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/smartregister/fhir/gateway/plugins/LocationHierarchyEndpointHelperTest.java
Outdated
Show resolved
Hide resolved
9323f53
to
1c1d783
Compare
Resolves #64
Engineer Checklist
bug fixes
option(s) on the
README.md
mvn spotless:check
to check my code follows the project'sstyle guide
mvn clean test jacoco:report
to confirm the coverage reportwas generated at
plugins/target/site/jacoco/index.html
mvn clean package
right before creating this pull request.