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.
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
AMM-871 Abdm Facility fetch services changes #46
AMM-871 Abdm Facility fetch services changes #46
Changes from 5 commits
3814860a2224b41a28c2ba985f425e6f9c84d37ef2File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Critical: Replace hardcoded development URL with environment variable.
The
getAbdmFacilityServiciesproperty has several issues:Apply this fix to use environment variables consistently:
Also, please note there's a typo in the property name ('Servicies' should be 'Services').
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.
🛠️ Refactor suggestion
Maintain consistent API versioning and naming conventions.
Two observations:
v1while related endpoints were changed tov0.5getAbdmFacilityServiciesshould begetAbdmFacilityServices📝 Committable suggestion
This file was deleted.
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.
🛠️ Refactor suggestion
Improve request parameter documentation and validation.
The current @param annotation uses a JSON string to define the schema, which could be better represented using a proper request DTO class. Additionally, validation for the new required fields should be added.
Consider refactoring to use a DTO:
💡 Codebase verification
ABDM facility validation is partially implemented but needs enhancement
The code shows basic null checks for ABDM facility parameters but lacks comprehensive validation against the ABDM facility registry:
LinkCareContext_NDHMServiceImplonly performs null checks:if(null != genOTP.getAbdmFacilityId() && null != genOTP.getAbdmFacilityName())@Value("${abdmFacilityId}")) but there's no validation logic to verify it against ABDM registry🔗 Analysis chain
Verify ABDM facility integration requirements.
The addition of
abdmFacilityIdandabdmFacilityNameparameters suggests integration with ABDM facility services. Ensure that these values are properly validated against the ABDM facility registry.Let me check the validation logic in the facility service implementation.
Let me check the NDHM service implementation for facility validation since the code shows integration with NDHM services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 1282
Script:
Length of output: 1291
Script:
Length of output: 2350
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.
Indents are all over the place @helenKaryamsetty
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.
🛠️ Refactor suggestion
Enhance error handling and response structure.
Several improvements can be made to this endpoint:
Apply these improvements:
@GetMapping(value = { "/getAbdmRegisteredFacilities" }) -public String getAbdmRegisteredFacilities(@RequestHeader(value = "Authorization") String Authorization) { +public ResponseEntity<FacilityResponse> getAbdmRegisteredFacilities( + @RequestHeader(value = "Authorization") String authorization) { + if (StringUtils.isBlank(authorization)) { + return ResponseEntity.badRequest().body( + new FacilityResponse(ErrorCodes.INVALID_AUTH_HEADER, "Authorization header is required")); + } OutputResponse response = new OutputResponse(); try { - String resp = facilityService.fetchRegisteredFacilities(); - response.setResponse(resp); + FacilityDTO resp = facilityService.fetchRegisteredFacilities(); + return ResponseEntity.ok(new FacilityResponse(resp)); } catch (FHIRException e) { - response.setError(5000, e.getMessage()); - logger.error(e.getMessage()); + logger.error("Failed to fetch registered facilities: {}", e.getMessage(), e); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body(new FacilityResponse(ErrorCodes.FACILITY_FETCH_ERROR, e.getMessage())); } - logger.info("Get ABDM Registered facilities API response" + response.toString()); - return response.toString(); }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.
Fix Swagger documentation and enhance endpoint implementation.
Several issues need attention:
Apply these improvements: