-
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
Various Enhancements and Fixes #23
Conversation
2d1d144
to
a0ec864
Compare
- Bump up version to 1.0.2 - Upgrade FHIR Common Utils version to 1.0.0 - Optimize location fetch - Fix null pointer for non Resource endpoint permission check - Migrate practitioner details endpoint to /PractitionerDetail - Fix InvalidRequestException handling causing interner server error - Update/Fix Allowed Queries Configuration - Clean up
a0ec864
to
55e38be
Compare
|
||
LocationHierarchyTree locationHierarchyTree = new LocationHierarchyTree(); | ||
LocationHierarchy locationHierarchy = new LocationHierarchy(); | ||
if (StringUtils.isNotBlank(locationId) && location != null) { | ||
if (location != null) { | ||
logger.info("Building Location Hierarchy of Location Id : " + locationId); | ||
locationHierarchyTree.buildTreeFromList(getLocationHierarchy(locationId, location)); |
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.
Need to test if this works correctly incase of locationId
is null, as are removing check on locationId
The check was added to ensure null-safety.
@ndegwamartin
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.
But, I re-visited this logic and I think it won't break the code @ndegwamartin
@@ -279,7 +280,7 @@ Pair<Composition, PractitionerDetails> fetchCompositionAndPractitionerDetails( | |||
requestBundle.addEntry( | |||
SyncAccessDecision.createBundleEntryComponent( | |||
Bundle.HTTPVerb.GET, | |||
"practitioner-details?keycloak-uuid=" + subject, | |||
"PractitionerDetail?keycloak-uuid=" + subject, |
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 have a doubt here, that PractitionerDetail
won't be accessible via the generic fhir client, since the API does not exist on the FHIR server. Rather, it is exposed on the Gateway server. Let me test this on my local with some data. @ndegwamartin
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.
@rehammuzzamil you are right. I didn't catch this during testing because:
a) We don't throw an exception if the practitioner details is absent (like we do for Composition)
b) We return a default blank Organization and/or CareTeam instead of an empty list here
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.
@rehammuzzamil this is now fixed!
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 have tested it out on my local, and it looks good to me. Thanks @ndegwamartin for the fix.
5a839d7
to
d5e72a2
Compare
- Refactor to optimize location hierarchy endpoint - Refactor to optimize practitioner details endpoint via cache - Clean up
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.
Approved
IMPORTANT: Where possible all PRs must be linked to a Github issue
Resolves #22
Engineer Checklist
bug fixes
mvn spotless:check
to check my code follows the project'sstyle guide
mvn clean test
to confirm all new and existing testspassed.
mvn clean package
right before creating this pull request.