Move code from Release 3.4.0 to Main #155
Conversation
* Update application.properties * Swagger changes * AMM-1456 : Skiped jwtToken validation if not present in Cookie or header from mobile request. * Jwtten and user-agent validation * null check * Constants added and null check * Empty check added * Optimized code * removed commented code * Constans and null check added (#134) * Update application.properties * Swagger changes * AMM-1456 : Skiped jwtToken validation if not present in Cookie or header from mobile request. * Jwtten and user-agent validation * null check * Constants added and null check * Empty check added * Optimized code * removed commented code * Exception Handled and Null conditions added * Coderabbit comments addressed * Prescription Template API not working (#139) * Update application.properties * Swagger changes * AMM-1456 : Skiped jwtToken validation if not present in Cookie or header from mobile request. * Jwtten and user-agent validation * null check * Constants added and null check * Empty check added * Optimized code * removed commented code * Exception Handled and Null conditions added * Coderabbit comments addressed * AMM-1473 * AMM-1473 NCDScreening confirmed API change (#144) * Update application.properties * Swagger changes * AMM-1456 : Skiped jwtToken validation if not present in Cookie or header from mobile request. * Jwtten and user-agent validation * null check * Constants added and null check * Empty check added * Optimized code * removed commented code * Exception Handled and Null conditions added * Coderabbit comments addressed * AMM-1473 * fix(bug): added high_risk to worklist to show flw highrisk patients (#145) Co-authored-by: KA40094929 <KA40094929@APL-5CD1394ZJT> * AMM-1733 * AMM-1733 High risk pregnency field added in nurse worklist (#146) * Update application.properties * Swagger changes * AMM-1456 : Skiped jwtToken validation if not present in Cookie or header from mobile request. * Jwtten and user-agent validation * null check * Constants added and null check * Empty check added * Optimized code * removed commented code * Exception Handled and Null conditions added * Coderabbit comments addressed * AMM-1473 * AMM-1733 --------- Co-authored-by: Helen Grace Karyamsetty <133211481+helenKaryamsetty@users.noreply.github.com> Co-authored-by: KA40094929 <KA40094929@APL-5CD1394ZJT>
Rebase from main to Release 3.4.0
WalkthroughAdds a CBAC-by-user endpoint and service/repo support; introduces transient is_high_risk with repo query and nurse-service enrichment; hardens CHO app sync and activity retrieval; conditions JWT forwarding; adds counsellingProvidedList and an investigation-wrapper method; migrates CbacDetails to Lombok and expands fields; bumps hwc-api version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as NCDScreeningController
participant Service as NCDScreeningServiceImpl
participant Repo as CbacDetailsRepo
participant DB
Client->>Controller: POST /ncd/cbac/getByUserCbacDetails { createdBy }
Controller->>Controller: parse & validate createdBy from Map
alt createdBy missing
Controller-->>Client: 400 Bad Request
else
Controller->>Service: getCbacData(createdBy)
Service->>Repo: findByCreatedBy(createdBy)
Repo->>DB: SELECT ... WHERE created_by = ?
DB-->>Repo: List<CbacDetails>
Repo-->>Service: List<CbacDetails>
Service->>Service: augment each item (beneficiaryId), serialize to JSON
Service-->>Controller: JSON string
Controller-->>Client: 200 OK (JSON)
end
opt error
note right of Service: on exception → log and return 500
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/hwc/controller/wo/LocationControllerWo.java (1)
46-46: Eliminate controller-level mutable state; keep OutputResponse request-scopedGood switch to a method-local OutputResponse here. Please remove the class field and apply this pattern across all endpoints to avoid cross-request races (Spring controllers are singletons).
- private OutputResponse response;Also applies to: 201-205
🧹 Nitpick comments (5)
src/main/java/com/iemr/hwc/utils/JwtUserIdValidationFilter.java (1)
11-11: Remove duplicate import statement.Line 11 contains a duplicate import for
AuthorizationHeaderRequestWrapperthat was already imported on Line 9.Apply this diff to remove the duplicate import:
-import com.iemr.hwc.utils.http.AuthorizationHeaderRequestWrapper;src/main/java/com/iemr/hwc/data/benFlowStatus/BeneficiaryFlowStatus.java (1)
300-301: Add @expose annotation for consistent serialization.The
is_high_riskfield is missing the@Exposeannotation while other transient fields in this class that are meant to be serialized have it. This inconsistency could cause the field to be excluded from JSON responses depending on Gson configuration.Apply this diff to add the missing annotation:
+@Expose @Transient Boolean is_high_risk;src/main/java/com/iemr/hwc/service/common/transaction/CommonNurseServiceImpl.java (1)
3544-3552: Handle null is_high_risk explicitly (default to false)getIsHighrisk(...) can return null; current code skips setIs_high_risk(...) and leaves the @transient Boolean is_high_risk as null, which can confuse downstream consumers/serializers.
- Quick fix (recommended): set explicit boolean in CommonNurseServiceImpl, e.g. beneficiaryFlowStatus.setIs_high_risk(Boolean.TRUE.equals(isHighrisk));
- Alternative: change BeneficiaryFlowStatus.setIs_high_risk to accept Boolean and initialize the field (or document null semantics).
Locations: src/main/java/com/iemr/hwc/service/common/transaction/CommonNurseServiceImpl.java (call) and src/main/java/com/iemr/hwc/data/benFlowStatus/BeneficiaryFlowStatus.java (field + setter)
src/main/java/com/iemr/hwc/controller/wo/LocationControllerWo.java (2)
206-213: Handle null/empty results and improve error logging without leaking internals
- If the service returns null/empty, return 204 (or 404 as per API contract) instead of serializing nulls.
- Log the exception with stack trace; don’t echo exception details to clients.
try { String resList = locationServiceImpl.getOutreachProgramsList(stateID); - response.setResponse(resList); + if (resList == null || resList.isEmpty()) { + statusCode = HttpStatus.NO_CONTENT; + response.setResponse("[]"); + } else { + response.setResponse(resList); + } } catch (Exception e) { - logger.error("Error while fetching outreach list for stateId" + stateID); - response.setError(500, "Unable to fetch outreach list for stateId" + stateID + "Exception - " + e); + logger.error("Error while fetching outreach list for stateId {}", stateID, e); + response.setError(500, "Unable to fetch outreach list for stateId " + stateID); statusCode = HttpStatus.INTERNAL_SERVER_ERROR; }
203-205: Use ResponseEntity builder with MediaType instead of manual headersCleaner and more idiomatic; also avoids manual MultiValueMap.
-MultiValueMap<String, String> headers = new LinkedMultiValueMap<>(); -headers.add("Content-Type", "application/json"); +// (headers no longer needed) -return new ResponseEntity<>(response.toStringWithSerializeNulls(), headers, statusCode); +return ResponseEntity + .status(statusCode) + .contentType(MediaType.APPLICATION_JSON) + .body(response.toStringWithSerializeNulls());Add import:
import org.springframework.http.MediaType;Also applies to: 214-214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pom.xml(2 hunks)src/main/java/com/iemr/hwc/controller/ncdscreening/NCDScreeningController.java(2 hunks)src/main/java/com/iemr/hwc/controller/wo/LocationControllerWo.java(1 hunks)src/main/java/com/iemr/hwc/data/benFlowStatus/BeneficiaryFlowStatus.java(2 hunks)src/main/java/com/iemr/hwc/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java(1 hunks)src/main/java/com/iemr/hwc/repo/nurse/ncdscreening/CbacDetailsRepo.java(1 hunks)src/main/java/com/iemr/hwc/service/choApp/CHOAppSyncServiceImpl.java(2 hunks)src/main/java/com/iemr/hwc/service/common/transaction/CommonNurseServiceImpl.java(1 hunks)src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningService.java(1 hunks)src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java(2 hunks)src/main/java/com/iemr/hwc/utils/JwtUserIdValidationFilter.java(2 hunks)src/main/java/com/iemr/hwc/utils/RestTemplateUtil.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/iemr/hwc/utils/JwtUserIdValidationFilter.java (1)
src/main/java/com/iemr/hwc/utils/http/AuthorizationHeaderRequestWrapper.java (1)
AuthorizationHeaderRequestWrapper(10-42)
src/main/java/com/iemr/hwc/utils/RestTemplateUtil.java (1)
src/main/java/com/iemr/hwc/utils/Constants.java (1)
Constants(3-9)
🔇 Additional comments (11)
src/main/java/com/iemr/hwc/utils/JwtUserIdValidationFilter.java (1)
119-120: Proper error handling enhancement.The formatting changes improve readability while maintaining the existing error handling behavior.
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningService.java (1)
48-48: LGTM - Consistent service interface extension.The new method signature follows the existing pattern and properly declares the IEMRException in the method signature.
pom.xml (1)
9-9: Version bump aligns with release objectives.The version update from 3.2.1 to 3.4.0 is consistent with the PR's objective of moving code from release-3.4.0 to main.
src/main/java/com/iemr/hwc/service/choApp/CHOAppSyncServiceImpl.java (4)
607-626: Enhanced error handling with proper exception details.The method now includes comprehensive try-catch handling that logs errors and returns meaningful status codes. The error message includes both the user ID and exception details for better debugging.
628-644: Improved error handling and success confirmation.Good enhancement with try-catch block that provides specific error context including both user ID and template ID, along with explicit success messaging.
685-705: Enhanced error handling for user activities retrieval.The method now includes proper exception handling with logging and returns structured error responses with appropriate HTTP status codes.
707-740: Excellent null-safe implementation with Optional handling.The method now properly uses Optional to handle potential null cases and includes null-safe image encoding. The approach prevents NPEs and provides clear 404 responses when activities are not found.
src/main/java/com/iemr/hwc/repo/nurse/ncdscreening/CbacDetailsRepo.java (1)
30-30: LGTM - Clean repository extension.The addition of the import and the new query method follows Spring Data JPA conventions and supports the new user-based CBAC retrieval functionality.
Also applies to: 38-38
src/main/java/com/iemr/hwc/utils/RestTemplateUtil.java (1)
42-43: Conditional JWT header forwarding improves security.The change from always adding a "JwtToken" header to conditionally forwarding the JWT_TOKEN header only when present is a good security practice. This prevents unnecessary header injection and follows the principle of least information exposure.
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java (2)
1700-1700: Direct casting is cleaner than BigInteger conversion.The change from
((BigInteger) obj[0]).longValue()to((Long) obj[0])simplifies the code and suggests the underlying query now returns Long directly, which is more appropriate for ID fields.
1900-1908: Well-implemented service method for user-based CBAC retrieval.The new method follows the established patterns in this class:
- Uses proper exception handling with IEMRException
- Leverages the repository method correctly
- Returns JSON-serialized response consistently
- Has appropriate @OverRide annotation
| @Operation(summary = "Get NCD screening beneficiary CBAC details") | ||
| @PostMapping(value = { "/getByUserCbacDetails" }) | ||
| public String getByUserCbacDetails( | ||
| @RequestBody Map<String, String> requestBody) { | ||
|
|
||
|
|
||
|
|
||
| OutputResponse response = new OutputResponse(); | ||
| try { | ||
| String createdBy = requestBody.get("createdBy"); | ||
|
|
||
| if (createdBy == null || createdBy.isEmpty()) { | ||
| response.setError(400, "createdBy' is required in request body"); | ||
|
|
||
| } | ||
| if (createdBy==null) | ||
| throw new IEMRException("invalid user"); | ||
| String screeningDetails = ncdScreeningService.getCbacData(createdBy); | ||
| response.setResponse(screeningDetails); | ||
| } catch (Exception e) { | ||
| response.setError(500, "Error while getting captured CBAC data "); | ||
| logger.error("Error while getting captured CBAC data :" + e); | ||
| } | ||
| return response.toStringWithSerializeNulls(); | ||
| } |
There was a problem hiding this comment.
Improve validation and error handling logic.
The current implementation has several issues:
- Redundant validation: Lines 402-405 set an error but don't return, allowing execution to continue
- Ineffective null check: Line 406-407 will never execute because
createdBywas already validated above - Inconsistent error codes: Using both 400 and 500 for similar validation scenarios
Apply this diff to fix the validation logic:
try {
String createdBy = requestBody.get("createdBy");
- if (createdBy == null || createdBy.isEmpty()) {
- response.setError(400, "createdBy' is required in request body");
-
- }
- if (createdBy==null)
- throw new IEMRException("invalid user");
+ if (createdBy == null || createdBy.isEmpty()) {
+ throw new IEMRException("'createdBy' is required in request body");
+ }
+
String screeningDetails = ncdScreeningService.getCbacData(createdBy);
response.setResponse(screeningDetails);
} catch (Exception e) {
- response.setError(500, "Error while getting captured CBAC data ");
+ response.setError(5000, "Error while getting captured CBAC data ");
logger.error("Error while getting captured CBAC data :" + e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Operation(summary = "Get NCD screening beneficiary CBAC details") | |
| @PostMapping(value = { "/getByUserCbacDetails" }) | |
| public String getByUserCbacDetails( | |
| @RequestBody Map<String, String> requestBody) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| String createdBy = requestBody.get("createdBy"); | |
| if (createdBy == null || createdBy.isEmpty()) { | |
| response.setError(400, "createdBy' is required in request body"); | |
| } | |
| if (createdBy==null) | |
| throw new IEMRException("invalid user"); | |
| String screeningDetails = ncdScreeningService.getCbacData(createdBy); | |
| response.setResponse(screeningDetails); | |
| } catch (Exception e) { | |
| response.setError(500, "Error while getting captured CBAC data "); | |
| logger.error("Error while getting captured CBAC data :" + e); | |
| } | |
| return response.toStringWithSerializeNulls(); | |
| } | |
| @Operation(summary = "Get NCD screening beneficiary CBAC details") | |
| @PostMapping(value = { "/getByUserCbacDetails" }) | |
| public String getByUserCbacDetails( | |
| @RequestBody Map<String, String> requestBody) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| String createdBy = requestBody.get("createdBy"); | |
| if (createdBy == null || createdBy.isEmpty()) { | |
| throw new IEMRException("'createdBy' is required in request body"); | |
| } | |
| String screeningDetails = ncdScreeningService.getCbacData(createdBy); | |
| response.setResponse(screeningDetails); | |
| } catch (Exception e) { | |
| response.setError(5000, "Error while getting captured CBAC data "); | |
| logger.error("Error while getting captured CBAC data :" + e); | |
| } | |
| return response.toStringWithSerializeNulls(); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/hwc/controller/ncdscreening/NCDScreeningController.java
around lines 391 to 415, the request validation and error handling for createdBy
is flawed: remove the redundant/ineffective null check and ensure you return
immediately with a 400 response when createdBy is missing or empty (do not
continue execution), only call ncdScreeningService.getCbacData(createdBy) when
createdBy is valid, and use consistent error codes (400 for client validation
errors); in the catch block keep a 500 response but include the exception
details in the logger message for diagnostics.
| public Boolean isIs_high_risk() { | ||
| return is_high_risk; | ||
| } | ||
|
|
||
| public void setIs_high_risk(boolean is_high_risk) { | ||
| this.is_high_risk = is_high_risk; | ||
| } |
There was a problem hiding this comment.
Fix naming convention and method signature issues.
The getter method name isIs_high_risk() is unconventional and confusing. Additionally, the setter method parameter is a primitive boolean while the field is Boolean (nullable), which could lead to auto-boxing issues and prevents setting the field to null.
Apply this diff to fix the naming and type consistency:
-public Boolean isIs_high_risk() {
+public Boolean getIs_high_risk() {
return is_high_risk;
}
-public void setIs_high_risk(boolean is_high_risk) {
+public void setIs_high_risk(Boolean is_high_risk) {
this.is_high_risk = is_high_risk;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Boolean isIs_high_risk() { | |
| return is_high_risk; | |
| } | |
| public void setIs_high_risk(boolean is_high_risk) { | |
| this.is_high_risk = is_high_risk; | |
| } | |
| public Boolean getIs_high_risk() { | |
| return is_high_risk; | |
| } | |
| public void setIs_high_risk(Boolean is_high_risk) { | |
| this.is_high_risk = is_high_risk; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/hwc/data/benFlowStatus/BeneficiaryFlowStatus.java
around lines 304-310, the getter name isIs_high_risk() is unconventional and the
setter uses a primitive boolean while the field is a nullable Boolean; rename
the getter to getIs_high_risk() (or a camelCase equivalent like
isHighRisk/getHighRisk consistently across the class) and change the setter
signature to accept a java.lang.Boolean parameter so the field can be set to
null without boxing issues.
| @Query(value = "SELECT is_high_risk from t_anc_visit t WHERE t.ben_id = :ben_id order by 1 desc limit 1",nativeQuery = true) | ||
| public Boolean getIsHighrisk(@Param("ben_id") Long ben_id); |
There was a problem hiding this comment.
Potential performance issue with unbounded query.
The query lacks an ORDER BY clause and uses LIMIT 1 without specifying which record to return when multiple ANC visits exist for a beneficiary. This could return inconsistent results. The AI summary mentions "order by 1 desc limit 1" but the actual code shows "order by 1 desc limit 1" where "1" refers to the first column (is_high_risk), which doesn't guarantee the most recent visit.
Consider ordering by a timestamp field to get the most recent ANC visit:
-@Query(value = "SELECT is_high_risk from t_anc_visit t WHERE t.ben_id = :ben_id order by 1 desc limit 1",nativeQuery = true)
+@Query(value = "SELECT is_high_risk from t_anc_visit t WHERE t.ben_id = :ben_id ORDER BY t.created_date DESC LIMIT 1",nativeQuery = true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Query(value = "SELECT is_high_risk from t_anc_visit t WHERE t.ben_id = :ben_id order by 1 desc limit 1",nativeQuery = true) | |
| public Boolean getIsHighrisk(@Param("ben_id") Long ben_id); | |
| @Query(value = "SELECT is_high_risk from t_anc_visit t WHERE t.ben_id = :ben_id ORDER BY t.created_date DESC LIMIT 1",nativeQuery = true) | |
| public Boolean getIsHighrisk(@Param("ben_id") Long ben_id); |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/iemr/hwc/service/generalOPD/GeneralOPDServiceImpl.java (2)
878-882: Fix NPE when 'investigation' is absent
wrapperBenInvestigationANCcan be null if the request lacksinvestigation, causing NPE ongetExternalInvestigations().Apply this diff:
WrapperBenInvestigationANC wrapperBenInvestigationANC = InputMapper.gson() .fromJson(requestOBJ.get("investigation"), WrapperBenInvestigationANC.class); // Save Prescription -prescriptionDetail.setExternalInvestigation(wrapperBenInvestigationANC.getExternalInvestigations()); +if (wrapperBenInvestigationANC != null) { + prescriptionDetail.setExternalInvestigation(wrapperBenInvestigationANC.getExternalInvestigations()); +}
1500-1503: Guard against NPE when reading externalInvestigations during update
wrapperBenInvestigationANCTMPcan be null ifinvestigationis not provided in the update payload.Apply this diff:
-if (prescriptionDetail != null) { - prescriptionDetail.setExternalInvestigation(wrapperBenInvestigationANCTMP.getExternalInvestigations()); +if (prescriptionDetail != null && wrapperBenInvestigationANCTMP != null) { + prescriptionDetail.setExternalInvestigation(wrapperBenInvestigationANCTMP.getExternalInvestigations()); prescriptionID = prescriptionDetail.getPrescriptionID(); }
🧹 Nitpick comments (1)
src/main/java/com/iemr/hwc/service/generalOPD/GeneralOPDServiceImpl.java (1)
1520-1536: Potential update of a transient PrescriptionDetail without IDIf
diagnosisis absent butinvestigation.counsellingProvidedListexists, a newPrescriptionDetailis created here without aprescriptionID. LaterupdatePrescription(prescriptionDetail)(Line 1539) may fail/no-op due to missing ID.
- Ensure
prescriptionDetailcarries a validprescriptionIDbefore callingupdatePrescription. Options:
- Fetch the existing prescription by
(beneficiaryRegID, visitCode)and set its ID.- Alternatively, gate the update: skip
updatePrescriptionwhen nodiagnosis/ID is present and treat as no-op for diagnosis, keeping success flags consistent.
Would you like me to draft a guarded update pattern here?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/hwc/data/anc/WrapperBenInvestigationANC.java(1 hunks)src/main/java/com/iemr/hwc/service/common/transaction/CommonDoctorServiceImpl.java(1 hunks)src/main/java/com/iemr/hwc/service/generalOPD/GeneralOPDServiceImpl.java(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T13:46:05.259Z
Learnt from: helenKaryamsetty
PR: PSMRI/HWC-API#153
File: src/main/java/com/iemr/hwc/service/anc/ANCServiceImpl.java:1189-1191
Timestamp: 2025-08-13T13:46:05.259Z
Learning: In ANCServiceImpl.java, the getBenANCDetailsFrmNurseANC method should return resMap.toString() to maintain consistency with other similar methods in the codebase like getBenCaseRecordFromDoctorQuickConsult, getBenCaseRecordFromDoctorCAC, etc. The calling pattern expects this format.
Applied to files:
src/main/java/com/iemr/hwc/service/generalOPD/GeneralOPDServiceImpl.java
🔇 Additional comments (4)
src/main/java/com/iemr/hwc/data/anc/WrapperBenInvestigationANC.java (1)
41-50: Field addition LGTMThe
counsellingProvidedListarray with getters/setters is consistent with downstream usage and naming. No issues.src/main/java/com/iemr/hwc/service/common/transaction/CommonDoctorServiceImpl.java (1)
545-550: New wrapper-returning method LGTMStraightforward mirror of the JSON method; null-handling is delegated to callers. No issues.
src/main/java/com/iemr/hwc/service/generalOPD/GeneralOPDServiceImpl.java (2)
897-910: Good fallback for counsellingProvidedList under investigationUsing StringJoiner and null checks is clean and consistent with existing “||” serialization.
1394-1414: Wrapper-based investigation enrichment LGTMCorrectly parses diagnosis to extract counselling list, populates the wrapper, and serializes it. Null-path handled by creating a new wrapper.
Bug/fix issue cbac details
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java (2)
29-29: Remove unused Lombok import.
lombok.Datais not used; drop the import to avoid confusion.Apply this diff:
-import lombok.Data;
267-279: Expose of beneficiaryId: confirm PII intent; prefer DTO if not strictly needed.
@Transient @Expose private Long beneficiaryId;will be serialized. Ensure this aligns with privacy requirements. If not strictly required by clients, remove@Exposeor return a DTO that contains only needed fields.src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java (1)
1901-1913: N+1 queries + duplicated repo calls; tighten JSON serialization and mark read-only.
- N+1:
getBeneficiaryIdis called per row; consider batching in the repo to avoid per-record DB hits.- Duplicate call: the getter is invoked twice for the same benRegId.
- Serialization: prefer
GsonBuilder().excludeFieldsWithoutExposeAnnotation()for consistency with other endpoints.- Method is read-only: mark
@Transactional(readOnly = true).Apply the minimal improvements below; then consider batching at repo level.
- @Override - public String getCbacData(String userName) throws IEMRException { + @Override + @Transactional(readOnly = true) + public String getCbacData(String userName) throws IEMRException { try { List<CbacDetails> cbac = cbacDetailsRepo.findByCreatedBy(userName); - cbac.forEach(cbacDetails -> { - if(cbacDetailsRepo.getBeneficiaryId(cbacDetails.getBeneficiaryRegId())!=null){ - cbacDetails.setBeneficiaryId(cbacDetailsRepo.getBeneficiaryId(cbacDetails.getBeneficiaryRegId())); - } - }); - return new Gson().toJson(cbac); + for (CbacDetails d : cbac) { + Long benId = cbacDetailsRepo.getBeneficiaryId(d.getBeneficiaryRegId()); + if (benId != null) { + d.setBeneficiaryId(benId); + } + } + return new GsonBuilder().excludeFieldsWithoutExposeAnnotation().create().toJson(cbac); } catch (Exception e) { throw new IEMRException(e.getLocalizedMessage()); } }Optional repo-level batching (recommended):
- In
CbacDetailsRepo, add a method to fetch all beneficiaryIds for a set of benRegIds in one call (e.g., returningList<Object[]>), then map in-service.Also confirm the controller enforces authorization so users can only fetch CBAC data they are allowed to view, and add pagination if result sets can grow large.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java(2 hunks)src/main/java/com/iemr/hwc/repo/nurse/ncdscreening/CbacDetailsRepo.java(1 hunks)src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/hwc/repo/nurse/ncdscreening/CbacDetailsRepo.java
🔇 Additional comments (1)
src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java (1)
26-26: Jakarta vs javax JPA: verify project-wide consistency.Automated scan in the sandbox failed to search repo files (ripgrep: "No files were searched"); cannot confirm mixed imports — ensure the project consistently uses either jakarta.persistence or javax.persistence.
Run locally to check:
rg -n -C2 --no-ignore -g '!**/target/**' 'import[[:space:]]+(javax|jakarta)\.persistence' echo echo "Counts:" echo -n "javax.persistence: "; rg -n --no-ignore -g '!**/target/**' 'import[[:space:]]+javax\.persistence' | wc -l echo -n "jakarta.persistence: "; rg -n --no-ignore -g '!**/target/**' 'import[[:space:]]+jakarta\.persistence' | wc -l
| for (Object[] obj : resultSet) { | ||
| map.put("beneficiaryRegId", obj[0] != null ? ((BigInteger) obj[0]).longValue() : null); | ||
| map.put("beneficiaryRegId", obj[0] != null ? ((Long) obj[0]) : null); | ||
| if (obj[1] != null && (Boolean) obj[1] == true) | ||
| response.add("Diabetes Mellitus"); |
There was a problem hiding this comment.
Unsafe cast to Long can throw ClassCastException (likely BigInteger).
Native/aggregate queries often return BigInteger for numeric columns. Casting obj[0] to Long will break at runtime.
Use a robust conversion:
- map.put("beneficiaryRegId", obj[0] != null ? ((Long) obj[0]) : null);
+ Object id0 = obj[0];
+ Long beneficiaryRegId = null;
+ if (id0 instanceof java.math.BigInteger) {
+ beneficiaryRegId = ((java.math.BigInteger) id0).longValue();
+ } else if (id0 instanceof Number) {
+ beneficiaryRegId = ((Number) id0).longValue();
+ }
+ map.put("beneficiaryRegId", beneficiaryRegId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (Object[] obj : resultSet) { | |
| map.put("beneficiaryRegId", obj[0] != null ? ((BigInteger) obj[0]).longValue() : null); | |
| map.put("beneficiaryRegId", obj[0] != null ? ((Long) obj[0]) : null); | |
| if (obj[1] != null && (Boolean) obj[1] == true) | |
| response.add("Diabetes Mellitus"); | |
| for (Object[] obj : resultSet) { | |
| Object id0 = obj[0]; | |
| Long beneficiaryRegId = null; | |
| if (id0 instanceof java.math.BigInteger) { | |
| beneficiaryRegId = ((java.math.BigInteger) id0).longValue(); | |
| } else if (id0 instanceof Number) { | |
| beneficiaryRegId = ((Number) id0).longValue(); | |
| } | |
| map.put("beneficiaryRegId", beneficiaryRegId); | |
| if (obj[1] != null && (Boolean) obj[1] == true) | |
| response.add("Diabetes Mellitus"); |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java
around lines 1699 to 1702, the code unsafely casts obj[0] to Long which can
throw ClassCastException (DB returns BigInteger/Integer/etc.); replace the
direct cast with a safe conversion that handles Number types (e.g., check if
obj[0] is an instance of Number and use ((Number)obj[0]).longValue(), or if null
leave null), and ensure you handle BigInteger specifically if needed so
beneficiaryRegId is set to a Long without risking ClassCastException.
add column in cbac getails
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java(2 hunks)src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java(6 hunks)
🔇 Additional comments (1)
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java (1)
1692-1694: Fix ClassCastException when readingbeneficiaryRegId.The native query still returns
BigInteger(or otherNumber) inobj[0], so casting directly toLongwill blow up at runtime. Please convert defensively before putting it in the map. This was already pointed out earlier and is still outstanding.- map.put("beneficiaryRegId", obj[0] != null ? ((Long) obj[0]) : null); + Object rawBenRegId = obj[0]; + Long beneficiaryRegId = null; + if (rawBenRegId instanceof BigInteger) { + beneficiaryRegId = ((BigInteger) rawBenRegId).longValue(); + } else if (rawBenRegId instanceof Number) { + beneficiaryRegId = ((Number) rawBenRegId).longValue(); + } + map.put("beneficiaryRegId", beneficiaryRegId);
| import lombok.Data; | ||
|
|
||
| @Entity | ||
| @Table(name = "t_cbacdetails") | ||
| @Data | ||
| public class CbacDetails { |
There was a problem hiding this comment.
Avoid Lombok @Data on this JPA entity
@Data introduces generated equals, hashCode, and toString methods that include every column. For a JPA entity this is risky: equality/hash are no longer identity-based, can break persistence context semantics, and eager stringification can trigger lazy loads. Previously the class relied on default identity semantics; this change alters behavior and can surface subtle bugs. Please switch to explicit Lombok annotations that only add accessors.
-import lombok.Data;
+import lombok.Getter;
+import lombok.Setter;
@Entity
@Table(name = "t_cbacdetails")
-@Data
+@Getter
+@Setter
public class CbacDetails {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import lombok.Data; | |
| @Entity | |
| @Table(name = "t_cbacdetails") | |
| @Data | |
| public class CbacDetails { | |
| import lombok.Getter; | |
| import lombok.Setter; | |
| @Entity | |
| @Table(name = "t_cbacdetails") | |
| @Getter | |
| @Setter | |
| public class CbacDetails { |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java around lines
29–34, remove the Lombok @Data annotation and replace it with accessor-only
annotations to avoid generated equals/hashCode/toString; specifically, delete
@Data and add @Getter and @Setter (and any no-arg constructor annotation you
need, e.g., @NoArgsConstructor) so the entity retains default identity-based
equals/hashCode and avoids accidental lazy-loading in toString; also update
imports accordingly.
add column in cbac getails
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java (1)
1894-1899: Avoid duplicate repository lookups inside the loop
getBeneficiaryIdis invoked twice per record, which means doubling the database/network work for this method. Cache the lookup result locally before the null check and reuse it.- cbac.forEach(cbacDetails -> { - if(cbacDetailsRepo.getBeneficiaryId(cbacDetails.getBeneficiaryRegId())!=null){ - cbacDetails.setBeneficiaryId(cbacDetailsRepo.getBeneficiaryId(cbacDetails.getBeneficiaryRegId())); + cbac.forEach(cbacDetails -> { + Long beneficiaryId = cbacDetailsRepo.getBeneficiaryId(cbacDetails.getBeneficiaryRegId()); + if (beneficiaryId != null) { + cbacDetails.setBeneficiaryId(beneficiaryId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java(5 hunks)
🔇 Additional comments (1)
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java (1)
1689-1697: Fix unsafe cast when reading beneficiaryRegIdCasting
obj[0]straight toLongwill still throw whenever the native query yields aBigInteger(very common for numeric columns). We need to funnel the value throughNumber.longValue()with an explicitBigIntegerguard so this endpoint stops crashing at runtime.- map.put("beneficiaryRegId", obj[0] != null ? ((Long) obj[0]) : null); + Long beneficiaryRegId = null; + if (obj[0] instanceof BigInteger) { + beneficiaryRegId = ((BigInteger) obj[0]).longValue(); + } else if (obj[0] instanceof Number) { + beneficiaryRegId = ((Number) obj[0]).longValue(); + } + map.put("beneficiaryRegId", beneficiaryRegId);
add column in cbac getails
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java (1)
1888-1901: Avoid N+1 repo calls and double invocation in loopYou call getBeneficiaryId per row and twice per iteration. Cache and batch.
Apply minimal fix to remove double call and null-guard:
- cbac.forEach(cbacDetails -> { - if(cbacDetailsRepo.getBeneficiaryId(cbacDetails.getBeneficiaryRegId())!=null){ - cbacDetails.setBeneficiaryId(cbacDetailsRepo.getBeneficiaryId(cbacDetails.getBeneficiaryRegId())); - } - }); + cbac.forEach(cbacDetails -> { + Long regId = cbacDetails.getBeneficiaryRegId(); + if (regId != null) { + Long benId = cbacDetailsRepo.getBeneficiaryId(regId); + if (benId != null) { + cbacDetails.setBeneficiaryId(benId); + } + } + });Optionally, batch-fetch to eliminate N+1:
// In repo: @Query("select c.beneficiaryRegId, b.id from Beneficiary b join SomeMap c on c.beneficiaryRegId = b.beneficiaryRegId where c.beneficiaryRegId in :ids") List<Object[]> findBenIdsByRegIds(@Param("ids") Collection<Long> ids); // In service: Map<Long,Long> map = repo.findBenIdsByRegIds(ids).stream().collect(toMap(r->(Long)r[0], r->(Long)r[1])); cbac.forEach(d -> d.setBeneficiaryId(map.get(d.getBeneficiaryRegId())));src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java (1)
272-291: Inconsistent field naming (UpperCamel vs lowerCamel)New fields start with uppercase (CbacOccupationalExposure, etc.) unlike existing cbac* fields. Consider lowerCamelCase for consistency; if API needs specific JSON keys, add @SerializedName.
Example:
@SerializedName("Cbac_OccupationalExposure") private String cbacOccupationalExposure;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java(2 hunks)src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java(5 hunks)
🔇 Additional comments (4)
src/main/java/com/iemr/hwc/service/ncdscreening/NCDScreeningServiceImpl.java (2)
362-362: Good: use of visitCode setter aligns with modelRenaming to setVisitCode matches the entity field name.
1688-1688: Fix unsafe cast: db numeric often returns BigIntegerDirect cast to Long can throw ClassCastException. Convert via Number/BigInteger.
Apply:
- map.put("beneficiaryRegId", obj[0] != null ? ((Long) obj[0]) : null); + Object id0 = obj[0]; + Long beneficiaryRegId = null; + if (id0 instanceof java.math.BigInteger) { + beneficiaryRegId = ((java.math.BigInteger) id0).longValue(); + } else if (id0 instanceof Number) { + beneficiaryRegId = ((Number) id0).longValue(); + } + map.put("beneficiaryRegId", beneficiaryRegId);src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java (2)
268-271: Transient enrichment field LGTMTransient beneficiaryId exposed for response only is appropriate.
29-29: Do not use Lombok @DaTa on JPA entities@DaTa generates equals/hashCode/toString across all fields and can break JPA identity semantics and trigger lazy loads.
Apply:
-import lombok.Data; +import lombok.Getter; +import lombok.Setter; @@ -@Data +@Getter +@SetterBased on learnings
Also applies to: 33-33
| @Expose | ||
| @Column(name = "Cbac_OccupationalExposure_score") | ||
| private String CbacOccupationalExposureScore; | ||
|
|
||
| public Integer getVanSerialNo() { | ||
| return vanSerialNo; | ||
| } | ||
| @Expose | ||
| @Column(name = "Cbac_BotheredProblem_last2weeks_score") | ||
| private String CbacBotheredProblemLast2weeksScore; | ||
|
|
||
| public void setVanSerialNo(Integer vanSerialNo) { | ||
| this.vanSerialNo = vanSerialNo; | ||
| } | ||
| @Expose | ||
| @Column(name = "Cbac_LittleInterest_Pleasure_score") | ||
| private String CbacLittleInterestPleasureScore; | ||
|
|
||
| public Integer getVanId() { | ||
| return vanId; | ||
| } | ||
| @Expose | ||
| @Column(name = "Cbac_Depressed_hopeless_score") | ||
| private String CbacDepressedhopelessScore; | ||
|
|
||
| public void setVanId(Integer vanId) { | ||
| this.vanId = vanId; | ||
| } | ||
| @Expose | ||
| @Column(name = "Cbac_DiscolorationSkin") | ||
| private String CbacDiscolorationSkinScore; | ||
|
|
||
| public String getVehicalNo() { | ||
| return vehicalNo; | ||
| } | ||
| @Expose | ||
| @Column(name = "Cbac_Cooking_Oil_score") | ||
| private String CbacCookingOilScore; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Score fields should be numeric (Integer), not String
Existing score fields in this entity use Integer; new ones are String. This risks type mismatches with DB and inconsistent JSON.
Apply:
- private String CbacOccupationalExposureScore;
+ private Integer CbacOccupationalExposureScore;
@@
- private String CbacBotheredProblemLast2weeksScore;
+ private Integer CbacBotheredProblemLast2weeksScore;
@@
- private String CbacLittleInterestPleasureScore;
+ private Integer CbacLittleInterestPleasureScore;
@@
- private String CbacDepressedhopelessScore;
+ private Integer CbacDepressedhopelessScore;
@@
- private String CbacCookingOilScore;
+ private Integer CbacCookingOilScore;
@@
- private String CbacFeelingDownScore;
+ private Integer CbacFeelingDownScore;Please confirm DB column types for these “_score” columns and adjust accordingly.
Also applies to: 321-324
🏁 Script executed:
#!/bin/bash
set -e
# Locate CbacDetails.java
file=$(fd CbacDetails.java)
echo "Analyzing $file"
# List all score fields and their declared types
rg -nP 'private\s+(Integer|string)\s+\w*Score' -C2 "$file"Length of output: 1335
Convert new score fields to Integer
All newly added “…Score” fields in CbacDetails.java (CbacOccupationalExposureScore, CbacBotheredProblemLast2weeksScore, CbacLittleInterestPleasureScore, CbacDepressedhopelessScore, CbacDiscolorationSkinScore, CbacCookingOilScore, CbacFeelingDownScore) are declared as String but must be Integer to match existing score mappings. Update their types accordingly and confirm the corresponding DB columns are integer types.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java around lines
296 to 319, the newly added “…Score” fields (CbacOccupationalExposureScore,
CbacBotheredProblemLast2weeksScore, CbacLittleInterestPleasureScore,
CbacDepressedhopelessScore, CbacDiscolorationSkinScore, CbacCookingOilScore and
the related CbacFeelingDownScore) are declared as String but must be Integer;
change their types from String to Integer, update any corresponding
getters/setters/constructors to use Integer, keep the @Expose and @Column
annotations but ensure the Java type is Integer, and verify the database columns
are integer types (alter column types/migrations if necessary) so mappings align
with existing score fields.
| @Expose | ||
| @Column(name = "Cbac_DiscolorationSkin") | ||
| private String CbacDiscolorationSkinScore; | ||
|
|
There was a problem hiding this comment.
Wrong column name for “Score” field maps to base column
CbacDiscolorationSkinScore is annotated with "Cbac_DiscolorationSkin" instead of its “_score” column, causing duplicate column mapping.
Apply:
- @Column(name = "Cbac_DiscolorationSkin")
- private String CbacDiscolorationSkinScore;
+ @Column(name = "Cbac_DiscolorationSkin_score")
+ private Integer CbacDiscolorationSkinScore;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Expose | |
| @Column(name = "Cbac_DiscolorationSkin") | |
| private String CbacDiscolorationSkinScore; | |
| @Expose | |
| @Column(name = "Cbac_DiscolorationSkin_score") | |
| private Integer CbacDiscolorationSkinScore; |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/hwc/data/ncdScreening/CbacDetails.java around lines
312 to 315, the field CbacDiscolorationSkinScore is incorrectly annotated with
@Column(name = "Cbac_DiscolorationSkin") which duplicates the base column
mapping; change the annotation to use the score column name (e.g., @Column(name
= "Cbac_DiscolorationSkin_score")) so this field maps to the correct database
column and avoids duplicate column mapping.
add column in cbac getails
|
5 participants
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Improvements
Chores