Release 3.8.2#268
Open
SauravBizbRolly wants to merge 17 commits into
Open
Conversation
…_3.8.2 Fix/cg incentive logic issue patch 3.8.2
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR updates incentive lookup contracts, timestamp sources, pagination, and duplicate-prevention logic across multiple FLW services. It also changes a leprosy follow-up repository return type and rewires village-level form submissions to create incentive records directly. ChangesIncentive logic changes
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 9
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/flw/service/impl/TBConfirmedCaseServiceImpl.java (2)
67-72: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winInstantiate
TBConfirmedCaseinside the loop
entityis created once before iterating, so every DTO mutates the same JPA entity. After the firstsave, the generated id stays on that instance and later iterations update the same row instead of inserting new ones; a multi-item request will keep only the last DTO’s data. Movenew TBConfirmedCase()into the loop and drop the redundant null check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/TBConfirmedCaseServiceImpl.java` around lines 67 - 72, The loop in TBConfirmedCaseServiceImpl is reusing the same TBConfirmedCase instance across multiple DTOs, so later saves overwrite the first row instead of creating separate records. Move the entity creation into the per-item iteration in the method that calls repository.save and checkIncentive, and remove the unnecessary null guard so each DTO gets a fresh TBConfirmedCase before mapping and persistence.
94-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
actualTreatmentCompletionDatefor the TB incentive windowcheckIncentivenow keys the payout offexpectedTreatmentCompletionDateand passes that same date as both start/end, so the saved record is anchored to the planned completion date with a zero-length window. This should use the actual completion date instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/TBConfirmedCaseServiceImpl.java` around lines 94 - 113, In TBConfirmedCaseServiceImpl.checkIncentive, the incentive window is being built from expectedTreatmentCompletionDate and passed as both the start and end, which anchors the payout to the planned date instead of the real completion. Update the logic to use actualTreatmentCompletionDate for the Timestamp values, and keep the existing incentiveForTbFollowUpIsDrTb and incentiveForTbFollowUp calls unchanged except for the corrected date source.
🧹 Nitpick comments (3)
src/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.java (1)
452-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale variable name after incentive rename.
CHILD_MOBILIZATION_SESSIONSnow holds the "PULSE_POLIO_SUPPORT" incentive master; the identifier is misleading and can confuse future maintainers of this and the sibling incentive-check methods (Filariasis, ORS) that follow the same naming pattern.✏️ Suggested rename
- IncentiveActivity CHILD_MOBILIZATION_SESSIONS = incentivesRepo.findIncentiveMasterByNameAndGroup("PULSE_POLIO_SUPPORT", GroupName.ACTIVITY.getDisplayName()); - if (CHILD_MOBILIZATION_SESSIONS != null) { - addAshaIncentiveRecord(CHILD_MOBILIZATION_SESSIONS, ashaId,startDate,endDate); + IncentiveActivity pulsePolioSupport = incentivesRepo.findIncentiveMasterByNameAndGroup("PULSE_POLIO_SUPPORT", GroupName.ACTIVITY.getDisplayName()); + if (pulsePolioSupport != null) { + addAshaIncentiveRecord(pulsePolioSupport, ashaId,startDate,endDate); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.java` at line 452, The local variable name in CampaignServiceImpl is stale and no longer matches the incentive it loads, which can mislead the related incentive-check logic. Rename CHILD_MOBILIZATION_SESSIONS in the method that calls incentivesRepo.findIncentiveMasterByNameAndGroup("PULSE_POLIO_SUPPORT", ...) to a name that reflects PULSE_POLIO_SUPPORT, and keep the sibling incentive-check methods consistent with the same naming pattern.src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java (1)
1322-1323: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
setUpdatedBycall.
record.setUpdatedBy(...)is invoked twice on consecutive lines; remove the redundant call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java` around lines 1322 - 1323, The `record.setUpdatedBy(...)` call in `DiseaseControlServiceImpl` is duplicated on consecutive lines; remove the redundant invocation and keep only one update using the `userService.getUserDetail(diseaseScreening.getUserId()).getUserName()` value so `record` is assigned once.src/main/java/com/iemr/flw/service/impl/VillageLevelFormServiceImpl.java (1)
289-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the three near-identical checker methods.
checkVhndIncentive,checkPhcMeetingIncentive, andcheckVhncIncentiveare identical except for the incentive-name literal ("VHND_PARTICIPATION","CLUSTER_MEETING","VHSNC_MEETING"). This triplication (which also largely mirrorscheckAndAddIncentives) invites drift. Extract a single parameterized helper.♻️ Proposed consolidation
- private void checkVhndIncentive(Timestamp startTimestamp, Integer userId){ - try { - IncentiveActivity activity = incentivesRepo.findIncentiveMasterByNameAndGroup("VHND_PARTICIPATION", GroupName.ACTIVITY.getDisplayName()); - if(activity!=null){ - ... - } - } catch (Exception e) { - logger.error("Process Incentive Exception: ", e); - } - } - - private void checkPhcMeetingIncentive(Timestamp startTimestamp, Integer userId){ - ... // "CLUSTER_MEETING" - } - - private void checkVhncIncentive(Timestamp startTimestamp, Integer userId){ - ... // "VHSNC_MEETING" - } + private void checkActivityIncentive(String incentiveName, Timestamp startTimestamp, Integer userId) { + try { + IncentiveActivity activity = incentivesRepo.findIncentiveMasterByNameAndGroup(incentiveName, GroupName.ACTIVITY.getDisplayName()); + if (activity != null) { + IncentiveActivityRecord record = recordRepo + .findRecordByActivityIdCreatedDateBenId(activity.getId(), startTimestamp, 0L, userId); + if (record == null) { + String userName = userRepo.getUserNamedByUserId(userId); + record = new IncentiveActivityRecord(); + record.setActivityId(activity.getId()); + record.setCreatedDate(startTimestamp); + record.setCreatedBy(userName); + record.setStartDate(startTimestamp); + record.setEndDate(startTimestamp); + record.setUpdatedDate(startTimestamp); + record.setUpdatedBy(userName); + record.setAshaId(userId); + record.setBenId(0L); + record.setAmount(Long.valueOf(activity.getRate())); + record.setIsEligible(true); + recordRepo.save(record); + } + } + } catch (Exception e) { + logger.error("Process Incentive Exception: ", e); + } + }Call sites then become e.g.
checkActivityIncentive("VHND_PARTICIPATION", vhndForm.getCreatedDate(), vhndForm.getUserId());. This also removes the repeated double lookup ofgetUserNamedByUserIdper record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/VillageLevelFormServiceImpl.java` around lines 289 - 393, The three methods `checkVhndIncentive`, `checkPhcMeetingIncentive`, and `checkVhncIncentive` are duplicated except for the incentive name literal, so replace them with one parameterized helper that takes the incentive key plus timestamp and userId. Move the shared lookup/save logic into that helper, keep the existing `recordRepo`/`incentivesRepo` flow intact, and update the call sites to pass `"VHND_PARTICIPATION"`, `"CLUSTER_MEETING"`, or `"VHSNC_MEETING"` as needed. While consolidating, also avoid repeating `userRepo.getUserNamedByUserId(userId)` by resolving the user once per record creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java`:
- Around line 356-393: The incentive threshold check in saveLeprosyFollowUp is
using leprosyFollowUpRepository.findByBenId(entity.getBenId()).size() before the
current follow-up is persisted, so the 6/12 visit logic can trigger one record
late. Update DiseaseControlServiceImpl so the
leprosyFollowUpRepository.save(entity) happens before the size-based incentive
checks, or otherwise include the pending entity in the count, and keep the
incentiveLogicService calls aligned with the saved follow-up state.
- Around line 1414-1417: The follow-up-0 branch in DiseaseControlServiceImpl is
dereferencing an optional treatment start date without a null guard, and it also
uses bitwise AND instead of short-circuit AND. Update the conditional around the
chronicDiseaseVisitEntity.getFollowUpNo() check to use &&, and only call
getTreatmentStartDate().atStartOfDay() after confirming the date is not null;
keep the logic localized to this branch so the Timestamp conversion only runs
when the date is present.
- Around line 1309-1313: The incentive dedup lookup in addIncentive() is using
diseaseScreening.getCreatedDate() instead of the screening-based timestamp, so
it can miss existing records and allow duplicates. Update the
recordRepo.findRecordByActivityIdCreatedDateBenId call to use screeningTimestamp
from diseaseScreening.getScreeningDate(), keeping the lookup key aligned with
the value stored on IncentiveActivityRecord and handling null screening dates
consistently.
- Around line 356-357: The follow-up save logic in DiseaseControlServiceImpl is
calling findByBenId(entity.getBenId()) multiple times and using overlapping
thresholds, which can create duplicate incentive records once the beneficiary
count reaches 12. Read the follow-up list once before saving, store the count in
a local variable, and update the checks in the relevant incentive branches so
the ranges are mutually exclusive (for example, separate the 6–11 and 12+
cases). Apply the same fix to the later threshold block in the same service
method so both incentive paths use the same pre-save count and non-overlapping
conditions.
- Around line 1391-1394: Guard the incentive lookup before dereferencing its id
in DiseaseControlServiceImpl’s checkIncentive flow:
findIncentiveMasterByNameAndGroup(...) can return null, so the logger.info using
incentiveActivity.getId() must be protected. Add a null check (or safe logging)
for incentiveActivity, and apply the same pattern to the related
incentiveActivityCG and incentiveActivityCGForNCDnewPatient lookups if they are
logged or used later so saveChronicDiseaseVisit cannot fail when a master row is
missing.
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java`:
- Around line 828-830: The State 8 aborted branch in MaternalHealthServiceImpl
should match the null-safety used elsewhere: guard ancVisit.getIsAborted()
against null before unboxing, and only call recordAncRelatedIncentive when
paiucdActivityCH is non-null. Update the conditional around the aborted-path
logic in the same block that uses ancVisit and paiucdActivityCH so
recordAncRelatedIncentive is never reached with a null incentive activity.
- Around line 662-683: The CG contraception checks in MaternalHealthServiceImpl
can throw a null pointer exception because ect.getContraceptionMethod() is
called directly in the POST PARTUM IUCD (PPIUCD), FEMALE STERILIZATION, and MALE
STERILIZATION comparisons. Update this branch to use a null-safe check in the
same style as the AM block, either by guarding ect.getContraceptionMethod()
before comparing or by using constant-first equality in the addIncenticeRecord
conditions inside this section. Keep the logic within the existing
getAnyContraceptionMethod() flow and apply the fix consistently to all three
method comparisons.
- Around line 731-732: The transactional annotation on addIncenticeRecord is
ineffective because it is a private helper invoked from within
MaternalHealthServiceImpl, so Spring will not apply proxy-based transaction
management here. Move the transaction boundary to the public method that calls
addIncenticeRecord, or extract this logic into a separate Spring bean with the
transaction on a public method, and remove `@Transactional` from the private
helper.
- Around line 734-758: Add a DB-level uniqueness guard in addIncenticeRecord:
the current local lockMap/synchronized approach in MaternalHealthServiceImpl is
not enough to prevent duplicate inserts across threads, and the private helper’s
`@Transactional` boundary does not protect the check-then-insert flow. Add a
unique constraint/index on the IncentiveActivityRecord fields used by
findRecordByActivityIdCreatedDateBenId and save (activityId, benId,
createdDate), then update addIncenticeRecord to treat duplicate-key failures
from recordRepo.save as a no-op instead of creating another record.
---
Outside diff comments:
In `@src/main/java/com/iemr/flw/service/impl/TBConfirmedCaseServiceImpl.java`:
- Around line 67-72: The loop in TBConfirmedCaseServiceImpl is reusing the same
TBConfirmedCase instance across multiple DTOs, so later saves overwrite the
first row instead of creating separate records. Move the entity creation into
the per-item iteration in the method that calls repository.save and
checkIncentive, and remove the unnecessary null guard so each DTO gets a fresh
TBConfirmedCase before mapping and persistence.
- Around line 94-113: In TBConfirmedCaseServiceImpl.checkIncentive, the
incentive window is being built from expectedTreatmentCompletionDate and passed
as both the start and end, which anchors the payout to the planned date instead
of the real completion. Update the logic to use actualTreatmentCompletionDate
for the Timestamp values, and keep the existing incentiveForTbFollowUpIsDrTb and
incentiveForTbFollowUp calls unchanged except for the corrected date source.
---
Nitpick comments:
In `@src/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.java`:
- Line 452: The local variable name in CampaignServiceImpl is stale and no
longer matches the incentive it loads, which can mislead the related
incentive-check logic. Rename CHILD_MOBILIZATION_SESSIONS in the method that
calls incentivesRepo.findIncentiveMasterByNameAndGroup("PULSE_POLIO_SUPPORT",
...) to a name that reflects PULSE_POLIO_SUPPORT, and keep the sibling
incentive-check methods consistent with the same naming pattern.
In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java`:
- Around line 1322-1323: The `record.setUpdatedBy(...)` call in
`DiseaseControlServiceImpl` is duplicated on consecutive lines; remove the
redundant invocation and keep only one update using the
`userService.getUserDetail(diseaseScreening.getUserId()).getUserName()` value so
`record` is assigned once.
In `@src/main/java/com/iemr/flw/service/impl/VillageLevelFormServiceImpl.java`:
- Around line 289-393: The three methods `checkVhndIncentive`,
`checkPhcMeetingIncentive`, and `checkVhncIncentive` are duplicated except for
the incentive name literal, so replace them with one parameterized helper that
takes the incentive key plus timestamp and userId. Move the shared lookup/save
logic into that helper, keep the existing `recordRepo`/`incentivesRepo` flow
intact, and update the call sites to pass `"VHND_PARTICIPATION"`,
`"CLUSTER_MEETING"`, or `"VHSNC_MEETING"` as needed. While consolidating, also
avoid repeating `userRepo.getUserNamedByUserId(userId)` by resolving the user
once per record creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c97281d-e863-4e8f-8b30-cbd83af7de20
📒 Files selected for processing (9)
src/main/java/com/iemr/flw/domain/iemr/TBConfirmedCase.javasrc/main/java/com/iemr/flw/repo/iemr/DiseaseLeprosyRepository.javasrc/main/java/com/iemr/flw/repo/iemr/LeprosyFollowUpRepository.javasrc/main/java/com/iemr/flw/service/MaaMeetingService.javasrc/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/TBConfirmedCaseServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/VillageLevelFormServiceImpl.java
Comment on lines
+356
to
+357
| if(!leprosyFollowUpRepository.findByBenId(entity.getBenId()).isEmpty()){ | ||
| if(leprosyFollowUpRepository.findByBenId(entity.getBenId()).size()>=6 && leprosyFollowUpRepository.findByBenId(entity.getBenId()).size()<=12){ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## DiseaseControlServiceImpl excerpt\n'
sed -n '330,410p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n## leprosy follow-up repository definition\n'
rg -n "interface .*LeprosyFollowUpRepository|findByBenId\\(" src/main/java -g '*Leprosy*' -g '*Repository*.java'
printf '\n## nearby incentive methods\n'
rg -n "incentiveForLeprosy(Paucibacillary|Multibacillary)Confirmed" src/main/javaRepository: PSMRI/FLW-API
Length of output: 6626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## saveLeprosyFollowUpData method body\n'
sed -n '345,395p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n## incentive service definitions/usages\n'
rg -n "incentiveForLeprosy(Paucibacillary|Multibacillary)Confirmed" src/main/java -g '*.java' -A 8 -B 4Repository: PSMRI/FLW-API
Length of output: 11178
Use one pre-save count and make the thresholds exclusive
findByBenId(entity.getBenId()) is checked before this follow-up is saved, so once a beneficiary already has 12 records, the next save satisfies both >=6 && <=12 and >=12 and creates both incentive records. Read the list once, then split the thresholds (>=6 && <12 vs >=12, or the intended rule). Same issue applies at 375-376.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java`
around lines 356 - 357, The follow-up save logic in DiseaseControlServiceImpl is
calling findByBenId(entity.getBenId()) multiple times and using overlapping
thresholds, which can create duplicate incentive records once the beneficiary
count reaches 12. Read the follow-up list once before saving, store the count in
a local variable, and update the checks in the relevant incentive branches so
the ranges are mutually exclusive (for example, separate the 6–11 and 12+
cases). Apply the same fix to the later threshold block in the same service
method so both incentive paths use the same pre-save count and non-overlapping
conditions.
Comment on lines
+356
to
393
| if(!leprosyFollowUpRepository.findByBenId(entity.getBenId()).isEmpty()){ | ||
| if(leprosyFollowUpRepository.findByBenId(entity.getBenId()).size()>=6 && leprosyFollowUpRepository.findByBenId(entity.getBenId()).size()<=12){ | ||
| Integer userId = userRepo.getUserIdByName(entity.getCreatedBy()); | ||
| IncentiveActivityRecord incentiveActivityRecord = | ||
| incentiveLogicService.incentiveForLeprosyPaucibacillaryConfirmed( | ||
| entity.getBenId(), | ||
| entity.getTreatmentEndDate(), | ||
| entity.getTreatmentEndDate(), | ||
| userId); | ||
|
|
||
| if (incentiveActivityRecord != null) { | ||
| logger.info("Incentive processed for Screening Leprosy successfully. RecordId={}", | ||
| incentiveActivityRecord.getId()); | ||
| } else { | ||
| logger.info("Incentive not created"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if(!leprosyFollowUpRepository.findByBenId(entity.getBenId()).isEmpty()){ | ||
| if(leprosyFollowUpRepository.findByBenId(entity.getBenId()).size()>=12){ | ||
| Integer userId = userRepo.getUserIdByName(entity.getCreatedBy()); | ||
| IncentiveActivityRecord incentiveActivityRecord = | ||
| incentiveLogicService.incentiveForLeprosyMultibacillaryConfirmed( | ||
| entity.getBenId(), | ||
| entity.getTreatmentEndDate(), | ||
| entity.getTreatmentEndDate(), | ||
| userId); | ||
|
|
||
| if (incentiveActivityRecord != null) { | ||
| logger.info("Incentive processed for Screening Leprosy successfully. RecordId={}", | ||
| incentiveActivityRecord.getId()); | ||
| } else { | ||
| logger.info("Incentive not created"); | ||
| } | ||
| } | ||
| } | ||
| return entity; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file first
ast-grep outline src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java --view expanded
# Read the relevant section with line numbers
sed -n '300,450p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
# Find saveLeprosyFollowUp and saveLeprosyFollowUpData call sites/usages
rg -n "saveLeprosyFollowUpData|saveLeprosyFollowUp\\(" src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java src/main/java -A 3 -B 3Repository: PSMRI/FLW-API
Length of output: 14854
Persist the follow-up before counting for incentives saveLeprosyFollowUpData() checks leprosyFollowUpRepository.findByBenId(entity.getBenId()).size() before leprosyFollowUpRepository.save(entity) runs in saveLeprosyFollowUp, so the 6/12 thresholds fire one visit late. Save first, or include the pending follow-up in the count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java`
around lines 356 - 393, The incentive threshold check in saveLeprosyFollowUp is
using leprosyFollowUpRepository.findByBenId(entity.getBenId()).size() before the
current follow-up is persisted, so the 6/12 visit logic can trigger one record
late. Update DiseaseControlServiceImpl so the
leprosyFollowUpRepository.save(entity) happens before the size-based incentive
checks, or otherwise include the pending entity in the count, and keep the
incentiveLogicService calls aligned with the saved follow-up state.
Comment on lines
1309
to
1313
| Timestamp screeningTimestamp = diseaseScreening.getScreeningDate() != null | ||
| ? new Timestamp(diseaseScreening.getScreeningDate().getTime()) | ||
| : null; | ||
| IncentiveActivityRecord record = recordRepo | ||
| .findRecordByActivityIdCreatedDateBenId(diseaseScreeningActivity.getId(), Timestamp.valueOf(diseaseScreening.getCreatedDate().toString()), diseaseScreening.getBenId().longValue()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the service and repository methods involved.
git ls-files | rg 'DiseaseControlServiceImpl\.java|IncentiveActivityRecord|recordRepo|DiseaseScreening|DiseaseScreeningActivity'
printf '\n--- DiseaseControlServiceImpl around target lines ---\n'
sed -n '1280,1345p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n--- repository usages ---\n'
rg -n "findRecordByActivityIdCreatedDateBenId|createdDate|screeningTimestamp|screeningDate|saveMalariaDisease" src/main/java
printf '\n--- repository definition ---\n'
rg -n "interface .*RecordRepo|findRecordByActivityIdCreatedDateBenId" src/main/javaRepository: PSMRI/FLW-API
Length of output: 47250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- saveMalariaDisease and related flow ---\n'
sed -n '1037,1088p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n--- addIncentive for malaria (full block) ---\n'
sed -n '1300,1335p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n--- IncentiveRecordRepo methods around lookup ---\n'
sed -n '1,90p' src/main/java/com/iemr/flw/repo/iemr/IncentiveRecordRepo.java
printf '\n--- ScreeningMalaria entity fields ---\n'
sed -n '1,170p' src/main/java/com/iemr/flw/domain/iemr/ScreeningMalaria.javaRepository: PSMRI/FLW-API
Length of output: 13928
Use screeningTimestamp in the incentive dedup lookup.
addIncentive() stores createdDate from screeningDate, but the existence check still queries with diseaseScreening.getCreatedDate() (now). That makes the dedup key depend on save time instead of the screening date, so an existing record can be missed and duplicated; null screening dates will also never match.
src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java:1313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java`
around lines 1309 - 1313, The incentive dedup lookup in addIncentive() is using
diseaseScreening.getCreatedDate() instead of the screening-based timestamp, so
it can miss existing records and allow duplicates. Update the
recordRepo.findRecordByActivityIdCreatedDateBenId call to use screeningTimestamp
from diseaseScreening.getScreeningDate(), keeping the lookup key aligned with
the value stored on IncentiveActivityRecord and handling null screening dates
consistently.
Comment on lines
1391
to
1394
| IncentiveActivity incentiveActivity = incentivesRepo.findIncentiveMasterByNameAndGroup("NCD_FOLLOWUP_TREATMENT", GroupName.NCD.getDisplayName()); | ||
| IncentiveActivity incentiveActivityCG = incentivesRepo.findIncentiveMasterByNameAndGroup("NCD_FOLLOWUP_TREATMENT", GroupName.ACTIVITY.getDisplayName()); | ||
| IncentiveActivity incentiveActivityCGForNCDnewPatient = incentivesRepo.findIncentiveMasterByNameAndGroup("NCD_NEW_PATIENT_MEDICATION_SUPPORT", GroupName.ACTIVITY.getDisplayName()); | ||
| logger.info("incentiveActivity:" + incentiveActivity.getId()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant region in the service
sed -n '1360,1425p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n--- repository search ---\n'
# Locate the repository method declaration/implementation
rg -n "findIncentiveMasterByNameAndGroup" src/main/java src/test/javaRepository: PSMRI/FLW-API
Length of output: 29516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the repository/interface that declares the lookup and inspect its signature.
repo_file="$(rg -l "findIncentiveMasterByNameAndGroup" src/main/java | head -n 1)"
printf 'repo_file=%s\n' "$repo_file"
if [ -n "${repo_file:-}" ]; then
sed -n '1,220p' "$repo_file"
fi
printf '\n--- surrounding service logic ---\n'
sed -n '1380,1410p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.javaRepository: PSMRI/FLW-API
Length of output: 4599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect class/method annotations and the caller context around saveChronicDiseaseVisit.
sed -n '1,120p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n--- save method region ---\n'
sed -n '1320,1410p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
printf '\n--- null-guard pattern search ---\n'
rg -n "if \\(.*!= null\\).*getId\\(|logger\\.info\\(\".*getId\\(" src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java src/main/java/com/iemr/flw/service/impl/*.javaRepository: PSMRI/FLW-API
Length of output: 8939
Guard the incentive lookup before logging its id (DiseaseControlServiceImpl.java:1391-1394). findIncentiveMasterByNameAndGroup(...) returns null when the master row is missing, so incentiveActivity.getId() will throw and abort checkIncentive/saveChronicDiseaseVisit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java`
around lines 1391 - 1394, Guard the incentive lookup before dereferencing its id
in DiseaseControlServiceImpl’s checkIncentive flow:
findIncentiveMasterByNameAndGroup(...) can return null, so the logger.info using
incentiveActivity.getId() must be protected. Add a null check (or safe logging)
for incentiveActivity, and apply the same pattern to the related
incentiveActivityCG and incentiveActivityCGForNCDnewPatient lookups if they are
logged or used later so saveChronicDiseaseVisit cannot fail when a master row is
missing.
Comment on lines
+1414
to
+1417
| if(matchFound & Integer.valueOf(0).equals(chronicDiseaseVisitEntity.getFollowUpNo()) ){ | ||
| LocalDateTime localDateTime = chronicDiseaseVisitEntity.getTreatmentStartDate().atStartOfDay(); | ||
|
|
||
| Timestamp treatmentStartDate = Timestamp.valueOf(localDateTime); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java"
echo "### Context around lines 1390-1425"
sed -n '1390,1425p' "$FILE" | cat -n
echo
echo "### Search for treatmentStartDate handling in this file"
rg -n "getTreatmentStartDate|treatmentStartDate|followUpNo == 0|followUpNo" "$FILE"Repository: PSMRI/FLW-API
Length of output: 3349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### Locate ChronicDiseaseVisitEntity definition and treatmentStartDate field"
fd -a "ChronicDiseaseVisitEntity.java" src || true
rg -n "class ChronicDiseaseVisitEntity|LocalDate treatmentStartDate|getTreatmentStartDate|setTreatmentStartDate" src/main/java src/test/java
echo
echo "### Surrounding code where treatmentStartDate is populated before this branch"
sed -n '1348,1398p' src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java | cat -nRepository: PSMRI/FLW-API
Length of output: 5792
Guard treatmentStartDate before dereferencing it
chronicDiseaseVisitEntity.getTreatmentStartDate() is optional here, so atStartOfDay() can throw when a follow-up-0 record has no start date. Add a null check before this branch; && should be used instead of & as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java`
around lines 1414 - 1417, The follow-up-0 branch in DiseaseControlServiceImpl is
dereferencing an optional treatment start date without a null guard, and it also
uses bitwise AND instead of short-circuit AND. Update the conditional around the
chronicDiseaseVisitEntity.getFollowUpNo() check to use &&, and only call
getTreatmentStartDate().atStartOfDay() after confirming the date is not null;
keep the logic localized to this branch so the Timestamp conversion only runs
when the date is present.
Comment on lines
+662
to
+683
| if (PPIUCDActivityCH != null) { | ||
| if (ect.getContraceptionMethod().equals("POST PARTUM IUCD (PPIUCD)")) { | ||
| addIncenticeRecord(ect, userId, PPIUCDActivityCH); | ||
|
|
||
| if (femaleSterilizationActivityCH != null) { | ||
| addIncenticeRecord(ect, userId, femaleSterilizationActivityCH); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| IncentiveActivity maleSterilizationActivityCH = | ||
| incentivesRepo.findIncentiveMasterByNameAndGroup("FP_MALE_STER", GroupName.ACTIVITY.getDisplayName()); | ||
|
|
||
| if (maleSterilizationActivityCH != null) { | ||
| addIncenticeRecord(ect, userId, maleSterilizationActivityCH); | ||
| if (femaleSterilizationActivityCH != null) { | ||
| if (ect.getContraceptionMethod().equals("FEMALE STERILIZATION")) { | ||
| addIncenticeRecord(ect, userId, femaleSterilizationActivityCH); | ||
|
|
||
| } | ||
| } | ||
|
|
||
|
|
||
| if (maleSterilizationActivityCH != null) { | ||
| if (ect.getContraceptionMethod().equals("MALE STERILIZATION")) { | ||
| addIncenticeRecord(ect, userId, maleSterilizationActivityCH); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Potential NPE: getContraceptionMethod() dereferenced without a null guard.
Unlike the AM block above (which guards ect.getContraceptionMethod() != null before equals), this CG branch is only gated by getAnyContraceptionMethod() == true and then calls ect.getContraceptionMethod().equals(...) at Lines 663, 671, and 679. If anyContraceptionMethod is true but contraceptionMethod is null, this throws an NPE. Prefer constant-first comparison or an explicit null check.
🛡️ Suggested guard (constant-first comparison)
- if (PPIUCDActivityCH != null) {
- if (ect.getContraceptionMethod().equals("POST PARTUM IUCD (PPIUCD)")) {
+ if (PPIUCDActivityCH != null) {
+ if ("POST PARTUM IUCD (PPIUCD)".equals(ect.getContraceptionMethod())) {
addIncenticeRecord(ect, userId, PPIUCDActivityCH);
}
}
- if (femaleSterilizationActivityCH != null) {
- if (ect.getContraceptionMethod().equals("FEMALE STERILIZATION")) {
+ if (femaleSterilizationActivityCH != null) {
+ if ("FEMALE STERILIZATION".equals(ect.getContraceptionMethod())) {
addIncenticeRecord(ect, userId, femaleSterilizationActivityCH);
}
}
- if (maleSterilizationActivityCH != null) {
- if (ect.getContraceptionMethod().equals("MALE STERILIZATION")) {
+ if (maleSterilizationActivityCH != null) {
+ if ("MALE STERILIZATION".equals(ect.getContraceptionMethod())) {
addIncenticeRecord(ect, userId, maleSterilizationActivityCH);
}
}📝 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.
Suggested change
| if (PPIUCDActivityCH != null) { | |
| if (ect.getContraceptionMethod().equals("POST PARTUM IUCD (PPIUCD)")) { | |
| addIncenticeRecord(ect, userId, PPIUCDActivityCH); | |
| if (femaleSterilizationActivityCH != null) { | |
| addIncenticeRecord(ect, userId, femaleSterilizationActivityCH); | |
| } | |
| } | |
| } | |
| IncentiveActivity maleSterilizationActivityCH = | |
| incentivesRepo.findIncentiveMasterByNameAndGroup("FP_MALE_STER", GroupName.ACTIVITY.getDisplayName()); | |
| if (maleSterilizationActivityCH != null) { | |
| addIncenticeRecord(ect, userId, maleSterilizationActivityCH); | |
| if (femaleSterilizationActivityCH != null) { | |
| if (ect.getContraceptionMethod().equals("FEMALE STERILIZATION")) { | |
| addIncenticeRecord(ect, userId, femaleSterilizationActivityCH); | |
| } | |
| } | |
| if (maleSterilizationActivityCH != null) { | |
| if (ect.getContraceptionMethod().equals("MALE STERILIZATION")) { | |
| addIncenticeRecord(ect, userId, maleSterilizationActivityCH); | |
| } | |
| } | |
| if (PPIUCDActivityCH != null) { | |
| if ("POST PARTUM IUCD (PPIUCD)".equals(ect.getContraceptionMethod())) { | |
| addIncenticeRecord(ect, userId, PPIUCDActivityCH); | |
| } | |
| } | |
| if (femaleSterilizationActivityCH != null) { | |
| if ("FEMALE STERILIZATION".equals(ect.getContraceptionMethod())) { | |
| addIncenticeRecord(ect, userId, femaleSterilizationActivityCH); | |
| } | |
| } | |
| if (maleSterilizationActivityCH != null) { | |
| if ("MALE STERILIZATION".equals(ect.getContraceptionMethod())) { | |
| addIncenticeRecord(ect, userId, maleSterilizationActivityCH); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java`
around lines 662 - 683, The CG contraception checks in MaternalHealthServiceImpl
can throw a null pointer exception because ect.getContraceptionMethod() is
called directly in the POST PARTUM IUCD (PPIUCD), FEMALE STERILIZATION, and MALE
STERILIZATION comparisons. Update this branch to use a null-safe check in the
same style as the AM block, either by guarding ect.getContraceptionMethod()
before comparing or by using constant-first equality in the addIncenticeRecord
conditions inside this section. Keep the logic within the existing
getAnyContraceptionMethod() flow and apply the fix consistently to all three
method comparisons.
Comment on lines
+731
to
732
| @Transactional | ||
| private void addIncenticeRecord(PNCVisit ect, Integer userId, IncentiveActivity antaraActivity) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Spring @transactional work on private methods or self-invoked methods?
💡 Result:
In the default Spring AOP proxy-based configuration, @Transactional does not work on private methods or when a method is self-invoked [1][2][3]. Private Methods By default, Spring uses proxy-based AOP to apply transactional behavior [2][3]. These proxies work by intercepting calls to the bean. Because private methods are not accessible to the proxy, they cannot be intercepted, and the @Transactional annotation is ignored [4][1][5]. As of Spring Framework 6.0, protected or package-visible methods can be made transactional for class-based proxies, but private methods remain unsupported in the standard proxy mode [6][2]. Self-Invocation Self-invocation occurs when a method within the same class calls another method on the same instance [3][7]. Because the call is executed directly on the target object (using the 'this' reference) rather than through the Spring-managed proxy, the transaction interceptor is bypassed, and the @Transactional annotation has no effect [2][3][5]. Workarounds To address these limitations while staying within the default proxy-based model, you can consider the following: 1. Self-Injection: Inject the bean into itself (using @Lazy injection) and call the transactional method through the injected proxy instance [8][5][9]. 2. Refactoring: Move the transactional logic into a separate service bean so that the call is intercepted properly [5][9]. 3. Programmatic Transactions: Use the TransactionTemplate to manage transactions programmatically, which does not rely on AOP interception [5]. 4. AspectJ Mode: Switch from the default proxy-based AOP to AspectJ mode [2][3]. AspectJ uses bytecode weaving instead of proxies, allowing it to intercept any method call, including private methods and internal calls within the same class [3][9].
Citations:
- 1: https://www.baeldung.com/spring-transactional-annotation-private-method
- 2: https://docs.enterprise.spring.io/spring-framework/reference/data-access/transaction/declarative/annotations.html
- 3: https://docs.spring.io/spring-framework/reference/6.2-SNAPSHOT/data-access/transaction/declarative/annotations.html
- 4: https://stackoverflow.com/questions/4396284/does-spring-transactional-attribute-work-on-a-private-method
- 5: https://stackoverflow.com/questions/54947816/saving-entities-in-private-methods
- 6: https://docs.spring.io/spring-framework/reference/data-access/transaction/declarative/annotations.html
- 7: https://stackoverflow.com/questions/23931698/spring-transactional-annotation-self-invocation
- 8: Method invocation in same class when both methods are @Transactional, but method 2 never executes spring-projects/spring-framework#27534
- 9: https://stackoverflow.com/questions/3423972/spring-transaction-method-call-by-the-method-within-the-same-class-does-not-wo
Move the transaction boundary off this private helper. Spring’s proxy-based @Transactional won’t apply here because addIncenticeRecord is private and called from the same class, so this annotation is ignored. Put the transaction on the public entry point or move this logic to a separate bean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java`
around lines 731 - 732, The transactional annotation on addIncenticeRecord is
ineffective because it is a private helper invoked from within
MaternalHealthServiceImpl, so Spring will not apply proxy-based transaction
management here. Move the transaction boundary to the public method that calls
addIncenticeRecord, or extract this logic into a separate Spring bean with the
transaction on a public method, and remove `@Transactional` from the private
helper.
Comment on lines
+734
to
+758
| String lockKey = antaraActivity.getId() + "_" + ect.getBenId() + "_" + ect.getPncDate(); | ||
|
|
||
| Object lock = lockMap.computeIfAbsent(lockKey, k -> new Object()); | ||
|
|
||
| synchronized (lock) { | ||
| IncentiveActivityRecord record = recordRepo | ||
| .findRecordByActivityIdCreatedDateBenId(antaraActivity.getId(), ect.getPncDate(), ect.getBenId()); | ||
|
|
||
| if (record == null) { | ||
| record = new IncentiveActivityRecord(); | ||
| record.setActivityId(antaraActivity.getId()); | ||
| record.setCreatedDate(ect.getPncDate()); | ||
| record.setCreatedBy(ect.getCreatedBy()); | ||
| record.setStartDate(ect.getPncDate()); | ||
| record.setEndDate(ect.getPncDate()); | ||
| record.setUpdatedDate(ect.getPncDate()); | ||
| record.setUpdatedBy(ect.getCreatedBy()); | ||
| record.setBenId(ect.getBenId()); | ||
| record.setAshaId(userId); | ||
| record.setAmount(Long.valueOf(antaraActivity.getRate())); | ||
| recordRepo.save(record); | ||
| } | ||
| } | ||
|
|
||
| lockMap.remove(lockKey, lock); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the service method, entity, repository, and any schema/migration references.
rg -n -A4 -B4 "lockMap|findRecordByActivityIdCreatedDateBenId|IncentiveActivityRecord|`@Transactional`|save\\(record\\)" src/main/java src/main/resources . || true
# Show the relevant section of the service file with line numbers.
sed -n '700,790p' src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java
# Inspect the entity for uniqueness annotations / fields.
fd -a "IncentiveActivityRecord.java" src/main/java
sed -n '1,260p' "$(fd -a "IncentiveActivityRecord.java" src/main/java | head -n 1)"
# Inspect the repository method signature.
fd -a "*IncentiveActivityRecord*Repository*.java" src/main/java
sed -n '1,220p' "$(fd -a "*IncentiveActivityRecord*Repository*.java" src/main/java | head -n 1)"
# Search migrations / schema / DDL for uniqueness or indexes on the relevant columns.
rg -n -i "incentive_activity_record|activity_id|ben_id|created_date|unique|index" src/main/resources db . || trueRepository: PSMRI/FLW-API
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- service slice ---'
sed -n '730,765p' src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java
echo '--- entity file path ---'
fd -a "IncentiveActivityRecord.java" src/main/java | head -n 5
entity_file="$(fd -a "IncentiveActivityRecord.java" src/main/java | head -n 1)"
echo '--- entity annotations/fields ---'
sed -n '1,260p' "$entity_file"
echo '--- repository file path ---'
fd -a "*IncentiveRecordRepo*.java" src/main/java | head -n 5
repo_file="$(fd -a "*IncentiveRecordRepo*.java" src/main/java | head -n 1)"
echo '--- repository methods ---'
sed -n '1,220p' "$repo_file"
echo '--- schema/migration hits ---'
rg -n -i "incentive_activity_record|unique|index|activity_id|ben_id|created_date" src/main/resources db . \
--glob '!**/target/**' --glob '!**/build/**' --glob '!**/dist/**' | head -n 200Repository: PSMRI/FLW-API
Length of output: 4166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- repository path ---'
fd -a --glob '*IncentiveRecordRepo*.java' src/main/java | head -n 5
repo_file="$(fd -a --glob '*IncentiveRecordRepo*.java' src/main/java | head -n 1)"
echo '--- repository contents ---'
sed -n '1,220p' "$repo_file"
echo '--- class annotations around MaternalHealthServiceImpl ---'
sed -n '1,120p' src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java
echo '--- schema / migration references for incentive_activity_record ---'
rg -n -i "incentive_activity_record|create table|alter table|unique|index|activity_id|ben_id|created_date" \
src/main/resources . \
--glob '!**/target/**' --glob '!**/build/**' --glob '!**/dist/**' | head -n 250Repository: PSMRI/FLW-API
Length of output: 26319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- callers of addIncenticeRecord ---'
rg -n -A4 -B4 "addIncenticeRecord\\(" src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java
echo '--- nearby public methods and transactional annotations ---'
sed -n '650,790p' src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java
echo '--- any unique constraint on IncentiveActivityRecord or its table in the codebase ---'
rg -n -i "`@UniqueConstraint`|uniqueConstraints|incentive_activity_record" src/main/java src/main/resources . \
--glob '!**/target/**' --glob '!**/build/**' --glob '!**/dist/**' | head -n 120Repository: PSMRI/FLW-API
Length of output: 13565
Add a DB-level uniqueness guard in addIncenticeRecord. The JVM lock is local and the lockMap.remove(...) pattern still allows another thread to create a fresh monitor for the same key while a request is queued on the old one. Since this helper is private/self-invoked, the @Transactional here doesn’t protect the check-then-insert path either. Add a unique constraint on (activity_id, ben_id, created_date) and treat duplicate-key as a no-op.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java`
around lines 734 - 758, Add a DB-level uniqueness guard in addIncenticeRecord:
the current local lockMap/synchronized approach in MaternalHealthServiceImpl is
not enough to prevent duplicate inserts across threads, and the private helper’s
`@Transactional` boundary does not protect the check-then-insert flow. Add a
unique constraint/index on the IncentiveActivityRecord fields used by
findRecordByActivityIdCreatedDateBenId and save (activityId, benId,
createdDate), then update addIncenticeRecord to treat duplicate-key failures
from recordRepo.save as a no-op instead of creating another record.
Comment on lines
+828
to
830
| if (ancVisit.getIsAborted()) { | ||
| recordAncRelatedIncentive(paiucdActivityCH, ancVisit); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing null checks in the State 8 aborted branch.
paiucdActivityCH (Line 822) can be null and is passed straight to recordAncRelatedIncentive, which dereferences incentiveActivity.getId() → NPE. Additionally ancVisit.getIsAborted() is unboxed directly, whereas the State 5 block guards getIsAborted() != null. Align this branch with the other checks.
🛡️ Suggested guard
- if (ancVisit.getIsAborted()) {
+ if (paiucdActivityCH != null && ancVisit.getIsAborted() != null
+ && ancVisit.getIsAborted()) {
recordAncRelatedIncentive(paiucdActivityCH, ancVisit);
}📝 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.
Suggested change
| if (ancVisit.getIsAborted()) { | |
| recordAncRelatedIncentive(paiucdActivityCH, ancVisit); | |
| } | |
| if (paiucdActivityCH != null && ancVisit.getIsAborted() != null | |
| && ancVisit.getIsAborted()) { | |
| recordAncRelatedIncentive(paiucdActivityCH, ancVisit); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java`
around lines 828 - 830, The State 8 aborted branch in MaternalHealthServiceImpl
should match the null-safety used elsewhere: guard ancVisit.getIsAborted()
against null before unboxing, and only call recordAncRelatedIncentive when
paiucdActivityCH is non-null. Update the conditional around the aborted-path
logic in the same block that uses ancVisit and paiucdActivityCH so
recordAncRelatedIncentive is never reached with a null incentive activity.
* fix duplicate incentive issue * fix duplicate incentive issue * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix child after 2 year gap after marriage * fix hbyc incentive logic * fix hbyc incentive logic * fix hbyc incentive logic * optmiz query for incentive logic for hbnc
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.java (1)
475-487: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid excluding the campaign end date from deduplication.
addAshaIncentiveRecordpassesendDate.atStartOfDay()as an exclusive upper bound, so records created onendDatenever match. If these campaign dates are inclusive, the last day is always open to duplicate incentive inserts; single-day campaigns are entirely missed. UseendDate.plusDays(1).atStartOfDay()or the intended month boundary instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.java` around lines 475 - 487, The deduplication window in addAshaIncentiveRecord is using endDate.atStartOfDay() as the upper bound, which excludes records created later on the campaign end date. Update the end-of-range calculation in CampaignServiceImpl.addAshaIncentiveRecord so the query covers the full inclusive campaign period, such as by using endDate.plusDays(1).atStartOfDay() or the correct month-boundary timestamp, while keeping the existing recordRepo.findRecordByActivityIdCreatedDateBenId lookup unchanged.src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java (1)
710-714: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame unguarded
getContraceptionMethod()NPE risk in the PPS/MiniLap branch.Inside
if (ect.getPncPeriod() == 1 || ...), Lines 712-713 callect.getContraceptionMethod().equals(...)with no null check. This is the same defect flagged for the CG block above but at a distinct location; ifcontraceptionMethodis null this throws. Prefer constant-first comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java` around lines 710 - 714, The PPS/MiniLap branch in MaternalHealthServiceImpl has the same null-safety issue in the contraception check: `ect.getContraceptionMethod().equals(...)` can throw when the value is null. Update the conditional inside the PNC period block to use a null-safe comparison with the string literals first, mirroring the fix used in the CG branch, and keep the rest of the sterilisation-date logic unchanged.src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java (1)
1314-1338: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDedup lookup and insert use different
benId, so ORS records will duplicate.The existence check at Line 1315 queries with the real
benId, but the new record is saved withrecord.setBenId(0L)(Line 1329). On the next save the lookup (still keyed by realbenId) will never match the previously inserted0Lrow, so a fresh record is created every time. Make both sides consistent — either look up by0Lor store the realbenId.🛠️ Option: key the lookup by the same value that is persisted
IncentiveActivityRecord record = recordRepo - .findRecordByActivityIdCreatedDateBenId(immunizationActivity.getId(), visitDate, benId,data.getUserId()); + .findRecordByActivityIdCreatedDateBenId(immunizationActivity.getId(), visitDate, 0L, data.getUserId());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java` around lines 1314 - 1338, The ORS deduplication logic in ChildCareServiceImpl is using different benId values for the lookup and the saved record, so repeated inserts will never match an existing row. Update the IncentiveActivityRecord flow so the key used in recordRepo.findRecordByActivityIdCreatedDateBenId matches the value set on the new record via record.setBenId, keeping the lookup and persistence consistent. Choose either the real benId or 0L, and apply the same value in both the existence check and the save path.src/main/java/com/iemr/flw/service/impl/CoupleServiceImpl.java (1)
254-283: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the child-gap thresholds across both incentive paths.
checkIncentiveForChildGapusesfirstAndSecondChildGap >= 2andmarriageFirstChildGap >= 3, whileregisterEligibleCoupleusesfirstAndSecondChildGap >= 3andmarriageFirstChildGap >= 2. That makes the incentive outcome depend on which path runs; one of these cutoffs is wrong.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/CoupleServiceImpl.java` around lines 254 - 283, The child-gap eligibility thresholds are inconsistent between the incentive flow in CoupleServiceImpl and the other child-gap check path, so the result changes depending on which method runs. Review the logic in checkIncentiveForChildGap and registerEligibleCouple, then make the first-and-second child gap and marriage-first-child-gap cutoffs match the intended business rule in both places, using the existing state-specific incentive branches and createIncentiveRecord flow unchanged.
🧹 Nitpick comments (4)
src/main/java/com/iemr/flw/service/impl/IFAFormSubmissionServiceImpl.java (1)
117-140: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
getUserIdByNamelookups — reuse the already-resolveduserId.
ifaFormSubmissionData.getUserId()is already set fromuserServiceRoleRepo.getUserIdByName(req.getUserName())at construction time (line 64). This method now callsgetUserIdByNameagain at line 124 for the lookup key, and a third time at line 135 forsetAshaId, all resolving the same value — 2 extra DB round-trips per record inside aforEachloop.♻️ Suggested fix
private void addIFAIncentive(IFAFormSubmissionData ifaFormSubmissionData, IncentiveActivity incentiveActivityAM) { DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd-MM-yyyy"); LocalDate ifaVisitDate = LocalDate.parse(ifaFormSubmissionData.getVisitDate(), formatter); Timestamp ifaVisitDateTimestamp = Timestamp.valueOf(ifaVisitDate.atStartOfDay()); - IncentiveActivityRecord incentiveActivityRecord = incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId(incentiveActivityAM.getId(),ifaVisitDateTimestamp,ifaFormSubmissionData.getBeneficiaryId(),userServiceRoleRepo.getUserIdByName(ifaFormSubmissionData.getUserName())); + Integer ashaId = ifaFormSubmissionData.getUserId(); + IncentiveActivityRecord incentiveActivityRecord = incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId(incentiveActivityAM.getId(),ifaVisitDateTimestamp,ifaFormSubmissionData.getBeneficiaryId(),ashaId); if(incentiveActivityRecord==null){ incentiveActivityRecord = new IncentiveActivityRecord(); incentiveActivityRecord.setActivityId(incentiveActivityAM.getId()); incentiveActivityRecord.setCreatedDate(ifaVisitDateTimestamp); incentiveActivityRecord.setStartDate(ifaVisitDateTimestamp); incentiveActivityRecord.setEndDate(ifaVisitDateTimestamp); incentiveActivityRecord.setUpdatedDate(ifaVisitDateTimestamp); incentiveActivityRecord.setUpdatedBy(ifaFormSubmissionData.getUserName()); incentiveActivityRecord.setCreatedBy(ifaFormSubmissionData.getUserName()); incentiveActivityRecord.setBenId(ifaFormSubmissionData.getBeneficiaryId()); - incentiveActivityRecord.setAshaId(userServiceRoleRepo.getUserIdByName(ifaFormSubmissionData.getUserName())); + incentiveActivityRecord.setAshaId(ashaId); incentiveActivityRecord.setAmount(Long.valueOf(incentiveActivityAM.getRate())); incentiveActivityRecord.setIsEligible(true); incentiveRecordRepo.save(incentiveActivityRecord); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/IFAFormSubmissionServiceImpl.java` around lines 117 - 140, The addIFAIncentive method is repeatedly calling userServiceRoleRepo.getUserIdByName for the same user value, causing redundant DB lookups. Update IFAFormSubmissionServiceImpl.addIFAIncentive to reuse the already-resolved userId from IFAFormSubmissionData (or resolve it once at the start of the method) and use that value for both the incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId lookup and IncentiveActivityRecord.setAshaId.src/main/java/com/iemr/flw/repo/iemr/IncentiveRecordRepo.java (1)
65-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
Sliceinstead ofPage— the caller never reads the total count.
IncentiveServiceImpl.getAllIncentivesByUserIdonly callspageResult.hasNext()in its paging loop and never usesgetTotalElements()/getTotalPages().Pagetriggers an extraCOUNT(*)query per page fetched;Sliceavoids it entirely while still supportinghasNext().♻️ Suggested change
- `@Query`("SELECT DISTINCT record FROM IncentiveActivityRecord record WHERE record.ashaId = :ashaId") - Page<IncentiveActivityRecord> findRecordsByAsha(`@Param`("ashaId") Integer ashaId, Pageable pageable); + `@Query`("SELECT DISTINCT record FROM IncentiveActivityRecord record WHERE record.ashaId = :ashaId") + Slice<IncentiveActivityRecord> findRecordsByAsha(`@Param`("ashaId") Integer ashaId, Pageable pageable);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/repo/iemr/IncentiveRecordRepo.java` around lines 65 - 66, The repository method `IncentiveRecordRepo.findRecordsByAsha` is returning `Page` even though `IncentiveServiceImpl.getAllIncentivesByUserId` only uses `hasNext()` and never needs total counts. Change the query method to return `Slice<IncentiveActivityRecord>` instead of `Page<IncentiveActivityRecord>`, and update any related call sites or imports so the paging loop in `getAllIncentivesByUserId` continues to work without triggering the extra count query.src/main/java/com/iemr/flw/service/impl/InfantServiceImpl.java (1)
100-117: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
getUserIdByNamelookup — resolve once and reuse.
getUserIdByName(createdBy)is now called at line 101 for the lookup key and again at line 113 forsetAshaId, both resolving the same value. SinceaddIsSncuIncentiveruns inside aforEachoverinfantList, this doubles the DB round-trips per iteration unnecessarily.♻️ Suggested fix
private void addIsSncuIncentive(IncentiveActivity activity, Long benId, String createdBy, Timestamp createdDate) { - IncentiveActivityRecord incentiveActivityRecord = incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId(activity.getId(),createdDate,benId,userServiceRoleRepo.getUserIdByName(createdBy)); + Integer ashaId = userServiceRoleRepo.getUserIdByName(createdBy); + IncentiveActivityRecord incentiveActivityRecord = incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId(activity.getId(),createdDate,benId,ashaId); if(incentiveActivityRecord==null){ incentiveActivityRecord = new IncentiveActivityRecord(); incentiveActivityRecord.setActivityId(activity.getId()); incentiveActivityRecord.setCreatedDate(createdDate); incentiveActivityRecord.setCreatedBy(createdBy); incentiveActivityRecord.setStartDate(createdDate); incentiveActivityRecord.setEndDate(createdDate); incentiveActivityRecord.setUpdatedDate(createdDate); incentiveActivityRecord.setUpdatedBy(createdBy); incentiveActivityRecord.setBenId(benId); - incentiveActivityRecord.setAshaId(userServiceRoleRepo.getUserIdByName(createdBy)); + incentiveActivityRecord.setAshaId(ashaId); incentiveActivityRecord.setAmount(Long.valueOf(activity.getRate())); incentiveRecordRepo.save(incentiveActivityRecord); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/InfantServiceImpl.java` around lines 100 - 117, `addIsSncuIncentive` in `InfantServiceImpl` is doing the same `userServiceRoleRepo.getUserIdByName(createdBy)` lookup twice per call, which adds unnecessary DB hits inside the `infantList` loop. Resolve the user ID once at the start of `addIsSncuIncentive`, reuse that value for both `incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId(...)` and `incentiveActivityRecord.setAshaId(...)`, and keep the rest of the record creation logic unchanged.src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java (1)
289-291: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftIncentive creation runs per page inside a read (
getHBNCDetails) path.
checkAndAddHbncIncentives(...)is invoked for every fetched page during a listing call, and it performs nestedpncVisits × ancVisitsiteration with repeatedbeneficiaryRepo.getDetailsByRegID(...)lookups. Triggering write-side incentive logic on each page of a GET is both a side-effect on a read and an O(pages·records²) hot path. Consider moving incentive generation out of the read path (or running it once, not per page).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java` around lines 289 - 291, The HBNC incentive generation is being triggered from the read-only getHBNCDetails flow on every page, which creates side effects and repeated expensive work. Move the call to checkAndAddHbncIncentives(...) out of the paging loop in ChildCareServiceImpl so it runs once outside the GET/listing path, or relocate it to a dedicated write/process method. Keep the pagination logic in getHBNCDetails focused only on fetching records, and ensure checkAndAddHbncIncentives and its beneficiaryRepo.getDetailsByRegID(...) lookups are not executed per page.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/iemr/flw/repo/iemr/IncentiveRecordRepo.java`:
- Around line 23-36: The lookup in
IncentiveRecordRepo.findRecordByActivityIdCreatedDateBenId now matches by
MONTH/YEAR instead of exact createdDate, which changes deduplication scope
across all callers. Update this repository method to preserve the intended
granularity, or if month-scoped dedup is desired, adjust the event-specific
callers that use findRecordByActivityIdCreatedDateBenId to pass and document a
month-based key consistently so they don’t treat separate incentives as
duplicates.
In `@src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java`:
- Around line 882-888: The `checkAndAddHbyncIncentives` flow in
`ChildCareServiceImpl` is unboxing `hbyc.getIfa_given()` directly, which can
throw an NPE when the flag is null. Update the conditional that checks
`eligibleHbycVisits` to use a null-safe Boolean comparison such as
`Boolean.TRUE.equals(hbyc.getIfa_given())`, and keep the existing
`createIncentiveRecordforHbyncVisit` call guarded by the same branch.
In `@src/main/java/com/iemr/flw/service/impl/CoupleServiceImpl.java`:
- Around line 365-390: The lockKey setup in CoupleServiceImpl currently calls
activity.getId() before checking activity for null, which can throw a
NullPointerException. Move the null guard to the start of the method (or return
early when activity is null) before any use of activity, including the lockKey
computation and the record lookup in the synchronized block. Keep the existing
logic in the same flow using the current symbols activity, lockMap, and
recordRepo, but ensure no dereference happens until after the null check.
In `@src/main/java/com/iemr/flw/service/impl/DeathReportsServiceImpl.java`:
- Around line 221-229: The incentive creation block in DeathReportsServiceImpl
is calling createIncentiveRecord twice and once before checking for a null
IncentiveActivity, which can cause an NPE and duplicate records. Update the
cdrList.forEach logic to only invoke createIncentiveRecord after the
incentivesRepo.findIncentiveMasterByNameAndGroup lookup has returned a non-null
result, and remove the redundant unguarded call so each CDR is processed once.
- Around line 219-220: Resolve the user lookup in registerCDR so it uses the
creator from each CDR instead of cdrList.get(0).getCreatedBy(), since cdrList
may contain mixed createdBy values. Update the logic around
DeathReportsServiceImpl.registerCDR and the cdrList processing to derive userId
from the current cdr being handled, and apply that per-item before creating
incentive records so each record is attributed to the correct user.
In `@src/main/java/com/iemr/flw/service/impl/DeliveryOutcomeServiceImpl.java`:
- Line 160: The conditional in DeliveryOutcomeServiceImpl uses
getIsJSYBenificiary() directly, which can throw a NullPointerException when the
Boolean is null. Update the check in the save flow to use a null-safe Boolean
comparison such as Boolean.TRUE.equals(...) around
deliveryOutcome.getIsJSYBenificiary(), so the JSY branch only runs when the flag
is explicitly true.
- Around line 144-157: The place-of-delivery check in DeliveryOutcomeServiceImpl
is effectively always true because the negated comparisons are joined with OR,
so the institutional-delivery incentive is being created even for excluded
delivery locations. Update the condition in the delivery processing block to
require all excluded values to fail (use AND across the negated equalsIgnoreCase
checks), and keep the incentive creation calls in the same conditional path so
only valid institutional deliveries reach createIncentiveRecordforJsy.
In `@src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java`:
- Around line 244-246: The page-processing loop in IncentiveServiceImpl is
returning an empty JSON array too early when a fetched page is empty, which can
discard items already collected in finalDtos. Update the empty-page handling in
the loop so it stops pagination with a break or equivalent and then returns the
accumulated finalDtos JSON, using the existing entities/finalDtos flow in the
method instead of immediately returning Collections.emptyList().
- Around line 295-299: The verified-user enrichment in IncentiveServiceImpl is
making the same userRepo.getUserRole(...) call twice and using .get(0) without
checking for results, which can fail the paged mapping loop. Update the
entry.getVerifiedByUserId() branch to fetch the role list once, reuse it for
both supervisorRole and verifiedByUserName, and guard against a null/empty
result before reading the first element (or skip setting those fields when no
role rows exist).
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java`:
- Line 782: The `MaternalHealthServiceImpl` logic is dereferencing
`ancVisit.getIsAborted()` as a primitive condition without a null check, unlike
the guarded usage already present later in the same method. Update the `if`
checks in the `ancVisit` handling flow so they first verify `getIsAborted()` is
not null before evaluating its boolean value, and apply the same fix to both
occurrences of this pattern in the method to keep the behavior consistent and
avoid NPEs.
---
Outside diff comments:
In `@src/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.java`:
- Around line 475-487: The deduplication window in addAshaIncentiveRecord is
using endDate.atStartOfDay() as the upper bound, which excludes records created
later on the campaign end date. Update the end-of-range calculation in
CampaignServiceImpl.addAshaIncentiveRecord so the query covers the full
inclusive campaign period, such as by using endDate.plusDays(1).atStartOfDay()
or the correct month-boundary timestamp, while keeping the existing
recordRepo.findRecordByActivityIdCreatedDateBenId lookup unchanged.
In `@src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java`:
- Around line 1314-1338: The ORS deduplication logic in ChildCareServiceImpl is
using different benId values for the lookup and the saved record, so repeated
inserts will never match an existing row. Update the IncentiveActivityRecord
flow so the key used in recordRepo.findRecordByActivityIdCreatedDateBenId
matches the value set on the new record via record.setBenId, keeping the lookup
and persistence consistent. Choose either the real benId or 0L, and apply the
same value in both the existence check and the save path.
In `@src/main/java/com/iemr/flw/service/impl/CoupleServiceImpl.java`:
- Around line 254-283: The child-gap eligibility thresholds are inconsistent
between the incentive flow in CoupleServiceImpl and the other child-gap check
path, so the result changes depending on which method runs. Review the logic in
checkIncentiveForChildGap and registerEligibleCouple, then make the
first-and-second child gap and marriage-first-child-gap cutoffs match the
intended business rule in both places, using the existing state-specific
incentive branches and createIncentiveRecord flow unchanged.
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java`:
- Around line 710-714: The PPS/MiniLap branch in MaternalHealthServiceImpl has
the same null-safety issue in the contraception check:
`ect.getContraceptionMethod().equals(...)` can throw when the value is null.
Update the conditional inside the PNC period block to use a null-safe comparison
with the string literals first, mirroring the fix used in the CG branch, and
keep the rest of the sterilisation-date logic unchanged.
---
Nitpick comments:
In `@src/main/java/com/iemr/flw/repo/iemr/IncentiveRecordRepo.java`:
- Around line 65-66: The repository method
`IncentiveRecordRepo.findRecordsByAsha` is returning `Page` even though
`IncentiveServiceImpl.getAllIncentivesByUserId` only uses `hasNext()` and never
needs total counts. Change the query method to return
`Slice<IncentiveActivityRecord>` instead of `Page<IncentiveActivityRecord>`, and
update any related call sites or imports so the paging loop in
`getAllIncentivesByUserId` continues to work without triggering the extra count
query.
In `@src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java`:
- Around line 289-291: The HBNC incentive generation is being triggered from the
read-only getHBNCDetails flow on every page, which creates side effects and
repeated expensive work. Move the call to checkAndAddHbncIncentives(...) out of
the paging loop in ChildCareServiceImpl so it runs once outside the GET/listing
path, or relocate it to a dedicated write/process method. Keep the pagination
logic in getHBNCDetails focused only on fetching records, and ensure
checkAndAddHbncIncentives and its beneficiaryRepo.getDetailsByRegID(...) lookups
are not executed per page.
In `@src/main/java/com/iemr/flw/service/impl/IFAFormSubmissionServiceImpl.java`:
- Around line 117-140: The addIFAIncentive method is repeatedly calling
userServiceRoleRepo.getUserIdByName for the same user value, causing redundant
DB lookups. Update IFAFormSubmissionServiceImpl.addIFAIncentive to reuse the
already-resolved userId from IFAFormSubmissionData (or resolve it once at the
start of the method) and use that value for both the
incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId lookup and
IncentiveActivityRecord.setAshaId.
In `@src/main/java/com/iemr/flw/service/impl/InfantServiceImpl.java`:
- Around line 100-117: `addIsSncuIncentive` in `InfantServiceImpl` is doing the
same `userServiceRoleRepo.getUserIdByName(createdBy)` lookup twice per call,
which adds unnecessary DB hits inside the `infantList` loop. Resolve the user ID
once at the start of `addIsSncuIncentive`, reuse that value for both
`incentiveRecordRepo.findRecordByActivityIdCreatedDateBenId(...)` and
`incentiveActivityRecord.setAshaId(...)`, and keep the rest of the record
creation logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d9139bb-53e1-41c1-ab7a-c3502ef16b19
📒 Files selected for processing (15)
src/main/java/com/iemr/flw/repo/identity/BeneficiaryRepo.javasrc/main/java/com/iemr/flw/repo/iemr/HbncVisitRepo.javasrc/main/java/com/iemr/flw/repo/iemr/IncentiveRecordRepo.javasrc/main/java/com/iemr/flw/service/impl/AdolescentHealthServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/CampaignServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/CoupleServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/DeathReportsServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/DeliveryOutcomeServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/IFAFormSubmissionServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/IncentiveLogicImpl.javasrc/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/InfantServiceImpl.javasrc/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/flw/service/impl/DiseaseControlServiceImpl.java
Comment on lines
+23
to
+36
| SELECT * | ||
| FROM db_iemr.incentive_activity_record r | ||
| WHERE r.activity_id = :id | ||
| AND r.ben_id = :benId | ||
| AND r.asha_id = :ashaId | ||
| AND MONTH(r.start_date) = MONTH(:createdDate) | ||
| AND YEAR(r.start_date) = YEAR(:createdDate) | ||
| LIMIT 1 | ||
| """, nativeQuery = true) | ||
| IncentiveActivityRecord findRecordByActivityIdCreatedDateBenId( | ||
| @Param("id") Long id, | ||
| @Param("createdDate") Timestamp createdDate, | ||
| @Param("benId") Long benId, | ||
| @Param("ashaId") Integer ashaId); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Month-based dedup changes the lookup granularity across many incentive flows. This overload is used from multiple event-specific callers, so replacing exact createdDate matching with MONTH(...)/YEAR(...) will treat any prior record for the same activity+ben+asha in that calendar month as a duplicate. That can suppress legitimately separate incentives; if this is intentional, the callers need to be updated to pass a month-scoped key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/repo/iemr/IncentiveRecordRepo.java` around lines
23 - 36, The lookup in
IncentiveRecordRepo.findRecordByActivityIdCreatedDateBenId now matches by
MONTH/YEAR instead of exact createdDate, which changes deduplication scope
across all callers. Update this repository method to preserve the intended
granularity, or if month-scoped dedup is desired, adjust the event-specific
callers that use findRecordByActivityIdCreatedDateBenId to pass and document a
month-based key consistently so they don’t treat separate incentives as
duplicates.
Comment on lines
+882
to
+888
| if(hbyc.getIfa_given() && eligibleHbycVisits.contains(hbyc.getVisit_day())){ | ||
| if(hbyncLactatingIronPacketActivityCH!=null){ | ||
| createIncentiveRecordforHbyncVisit(hbyc, hbyc.getBeneficiaryId(), hbyncLactatingIronPacketActivityCH, hbyc.getCreated_by()); | ||
|
|
||
| if (hbyncOrsPacketActivityCH != null) { | ||
| if (hbyc.getOrs_given()) { | ||
| createIncentiveRecordforHbyncOrsDistribution(hbyc, hbyc.getBeneficiaryId(), hbyncOrsPacketActivityCH, hbyc.getCreated_by()); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
getIfa_given() unboxed without a null guard → possible NPE.
if(hbyc.getIfa_given() && ...) unboxes a Boolean that may be null for records where the flag was never set, throwing an NPE that aborts checkAndAddHbyncIncentives. Use Boolean.TRUE.equals(hbyc.getIfa_given()).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java` around
lines 882 - 888, The `checkAndAddHbyncIncentives` flow in `ChildCareServiceImpl`
is unboxing `hbyc.getIfa_given()` directly, which can throw an NPE when the flag
is null. Update the conditional that checks `eligibleHbycVisits` to use a
null-safe Boolean comparison such as `Boolean.TRUE.equals(hbyc.getIfa_given())`,
and keep the existing `createIncentiveRecordforHbyncVisit` call guarded by the
same branch.
Comment on lines
+365
to
+390
| String lockKey = activity.getId() + "_" + eligibleCoupleDTO.getBenId() + "_" + eligibleCoupleDTO.getCreatedDate(); | ||
|
|
||
| Object lock = lockMap.computeIfAbsent(lockKey, k -> new Object()); | ||
| synchronized (lock){ | ||
| if (activity != null) { | ||
| Integer userId = userRepo.getUserIdByName(eligibleCoupleDTO.getCreatedBy()); | ||
|
|
||
| IncentiveActivityRecord record = recordRepo | ||
| .findRecordByActivityIdCreatedDateBenId(activity.getId(), eligibleCoupleDTO.getCreatedDate(), eligibleCoupleDTO.getBenId(),userId); | ||
| if (record == null) { | ||
| record = new IncentiveActivityRecord(); | ||
| record.setActivityId(activity.getId()); | ||
| record.setCreatedDate(eligibleCoupleDTO.getCreatedDate()); | ||
| record.setCreatedBy(eligibleCoupleDTO.getCreatedBy()); | ||
| record.setStartDate(eligibleCoupleDTO.getCreatedDate()); | ||
| record.setEndDate(eligibleCoupleDTO.getCreatedDate()); | ||
| record.setUpdatedDate(eligibleCoupleDTO.getCreatedDate()); | ||
| record.setUpdatedBy(eligibleCoupleDTO.getCreatedBy()); | ||
| record.setBenId(eligibleCoupleDTO.getBenId()); | ||
| record.setAshaId(userId); | ||
| record.setAmount(Long.valueOf(activity.getRate())); | ||
| recordRepo.save(record); | ||
| } | ||
| } | ||
| } | ||
| lockMap.remove(lockKey, lock); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
activity.getId() dereferenced before the null check → NPE.
lockKey at Line 365 calls activity.getId(), but the if (activity != null) guard is only at Line 369. Several callers pass an activity that may be null (e.g. Lines 141, 150, 270, 286 forward the result of findIncentiveMasterByNameAndGroup without a prior null check), so this will NPE before the guard is reached. Move the null check to the top (early return). Also note lockMap.remove at Line 390 runs outside the synchronized block — same racy pattern flagged in IncentiveServiceImpl.
🛡️ Suggested guard
private void createIncentiveRecord(EligibleCoupleRegister eligibleCoupleDTO, IncentiveActivity activity) {
+ if (activity == null) {
+ return;
+ }
String lockKey = activity.getId() + "_" + eligibleCoupleDTO.getBenId() + "_" + eligibleCoupleDTO.getCreatedDate();
Object lock = lockMap.computeIfAbsent(lockKey, k -> new Object());
synchronized (lock){
- if (activity != null) {
Integer userId = userRepo.getUserIdByName(eligibleCoupleDTO.getCreatedBy());
...
- }
}
lockMap.remove(lockKey, lock);
}📝 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.
Suggested change
| String lockKey = activity.getId() + "_" + eligibleCoupleDTO.getBenId() + "_" + eligibleCoupleDTO.getCreatedDate(); | |
| Object lock = lockMap.computeIfAbsent(lockKey, k -> new Object()); | |
| synchronized (lock){ | |
| if (activity != null) { | |
| Integer userId = userRepo.getUserIdByName(eligibleCoupleDTO.getCreatedBy()); | |
| IncentiveActivityRecord record = recordRepo | |
| .findRecordByActivityIdCreatedDateBenId(activity.getId(), eligibleCoupleDTO.getCreatedDate(), eligibleCoupleDTO.getBenId(),userId); | |
| if (record == null) { | |
| record = new IncentiveActivityRecord(); | |
| record.setActivityId(activity.getId()); | |
| record.setCreatedDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setCreatedBy(eligibleCoupleDTO.getCreatedBy()); | |
| record.setStartDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setEndDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setUpdatedDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setUpdatedBy(eligibleCoupleDTO.getCreatedBy()); | |
| record.setBenId(eligibleCoupleDTO.getBenId()); | |
| record.setAshaId(userId); | |
| record.setAmount(Long.valueOf(activity.getRate())); | |
| recordRepo.save(record); | |
| } | |
| } | |
| } | |
| lockMap.remove(lockKey, lock); | |
| if (activity == null) { | |
| return; | |
| } | |
| String lockKey = activity.getId() + "_" + eligibleCoupleDTO.getBenId() + "_" + eligibleCoupleDTO.getCreatedDate(); | |
| Object lock = lockMap.computeIfAbsent(lockKey, k -> new Object()); | |
| synchronized (lock){ | |
| Integer userId = userRepo.getUserIdByName(eligibleCoupleDTO.getCreatedBy()); | |
| IncentiveActivityRecord record = recordRepo | |
| .findRecordByActivityIdCreatedDateBenId(activity.getId(), eligibleCoupleDTO.getCreatedDate(), eligibleCoupleDTO.getBenId(),userId); | |
| if (record == null) { | |
| record = new IncentiveActivityRecord(); | |
| record.setActivityId(activity.getId()); | |
| record.setCreatedDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setCreatedBy(eligibleCoupleDTO.getCreatedBy()); | |
| record.setStartDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setEndDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setUpdatedDate(eligibleCoupleDTO.getCreatedDate()); | |
| record.setUpdatedBy(eligibleCoupleDTO.getCreatedBy()); | |
| record.setBenId(eligibleCoupleDTO.getBenId()); | |
| record.setAshaId(userId); | |
| record.setAmount(Long.valueOf(activity.getRate())); | |
| recordRepo.save(record); | |
| } | |
| } | |
| lockMap.remove(lockKey, lock); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/CoupleServiceImpl.java` around lines
365 - 390, The lockKey setup in CoupleServiceImpl currently calls
activity.getId() before checking activity for null, which can throw a
NullPointerException. Move the null guard to the start of the method (or return
early when activity is null) before any use of activity, including the lockKey
computation and the record lookup in the synchronized block. Keep the existing
logic in the same flow using the current symbols activity, lockMap, and
recordRepo, but ensure no dereference happens until after the null check.
Comment on lines
+219
to
+220
| if(!cdrList.isEmpty()){ | ||
| Integer userId = userRepo.getUserIdByName(cdrList.get(0).getCreatedBy()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/java/com/iemr/flw/service/impl/DeathReportsServiceImpl.java"
echo "== outline =="
ast-grep outline "$FILE" --view expanded || true
echo "== lines around 180-280 =="
sed -n '180,280p' "$FILE"
echo "== search cdrList and createdBy usages =="
rg -n "cdrList|createdBy|getUserIdByName|ashaId" src/main/java/com/iemr/flw/service/impl/DeathReportsServiceImpl.javaRepository: PSMRI/FLW-API
Length of output: 5319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/java/com/iemr/flw/service/impl/DeathReportsServiceImpl.java"
sed -n '48,82p' "$FILE"Repository: PSMRI/FLW-API
Length of output: 1339
Resolve userId per cdr. registerCDR() builds cdrList from multiple DTOs, so createdBy can differ within the batch. Using cdrList.get(0).getCreatedBy() will misattribute incentive records whenever a batch contains mixed creators.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DeathReportsServiceImpl.java` around
lines 219 - 220, Resolve the user lookup in registerCDR so it uses the creator
from each CDR instead of cdrList.get(0).getCreatedBy(), since cdrList may
contain mixed createdBy values. Update the logic around
DeathReportsServiceImpl.registerCDR and the cdrList processing to derive userId
from the current cdr being handled, and apply that per-item before creating
incentive records so each record is attributed to the correct user.
Comment on lines
+221
to
+229
| cdrList.forEach(cdr -> { | ||
| IncentiveActivity immunizationActivity = | ||
| incentivesRepo.findIncentiveMasterByNameAndGroup("CHILD_DEATH_REPORTING", GroupName.CHILD_HEALTH.getDisplayName()); | ||
| createIncentiveRecord(cdr, cdr.getBenId(), userId, immunizationActivity); | ||
| if(immunizationActivity!=null){ | ||
| createIncentiveRecord(cdr,cdr.getBenId(),userId,immunizationActivity); | ||
|
|
||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
NPE + duplicate incentive creation: guard the first call and drop the redundant one.
createIncentiveRecord(cdr, cdr.getBenId(), userId, immunizationActivity) on Line 224 runs before the null check, and createIncentiveRecord immediately dereferences immunizationActivity.getId(). When findIncentiveMasterByNameAndGroup(...) returns null, this throws an NPE. The call is also duplicated by the guarded invocation on Line 226, so the record is processed twice. Remove the unguarded Line 224 and keep only the guarded call.
🐛 Proposed fix
cdrList.forEach(cdr -> {
IncentiveActivity immunizationActivity =
incentivesRepo.findIncentiveMasterByNameAndGroup("CHILD_DEATH_REPORTING", GroupName.CHILD_HEALTH.getDisplayName());
- createIncentiveRecord(cdr, cdr.getBenId(), userId, immunizationActivity);
if(immunizationActivity!=null){
createIncentiveRecord(cdr,cdr.getBenId(),userId,immunizationActivity);
}
});📝 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.
Suggested change
| cdrList.forEach(cdr -> { | |
| IncentiveActivity immunizationActivity = | |
| incentivesRepo.findIncentiveMasterByNameAndGroup("CHILD_DEATH_REPORTING", GroupName.CHILD_HEALTH.getDisplayName()); | |
| createIncentiveRecord(cdr, cdr.getBenId(), userId, immunizationActivity); | |
| if(immunizationActivity!=null){ | |
| createIncentiveRecord(cdr,cdr.getBenId(),userId,immunizationActivity); | |
| } | |
| }); | |
| cdrList.forEach(cdr -> { | |
| IncentiveActivity immunizationActivity = | |
| incentivesRepo.findIncentiveMasterByNameAndGroup("CHILD_DEATH_REPORTING", GroupName.CHILD_HEALTH.getDisplayName()); | |
| if(immunizationActivity!=null){ | |
| createIncentiveRecord(cdr,cdr.getBenId(),userId,immunizationActivity); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DeathReportsServiceImpl.java` around
lines 221 - 229, The incentive creation block in DeathReportsServiceImpl is
calling createIncentiveRecord twice and once before checking for a null
IncentiveActivity, which can cause an NPE and duplicate records. Update the
cdrList.forEach logic to only invoke createIncentiveRecord after the
incentivesRepo.findIncentiveMasterByNameAndGroup lookup has returned a non-null
result, and remove the redundant unguarded call so each CDR is processed once.
Comment on lines
+144
to
157
| if (placeOfDelivery != null && | ||
| (!placeOfDelivery.equalsIgnoreCase("home") || | ||
| !placeOfDelivery.equalsIgnoreCase("in transit") || | ||
| !placeOfDelivery.equalsIgnoreCase("other private hospital"))) { | ||
|
|
||
| // Institutional delivery (eligible case) | ||
| if (institutionalDeliveryActivityAM != null) { | ||
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityAM); | ||
| } | ||
|
|
||
| delOutList.forEach(deliveryOutcome -> { | ||
|
|
||
| IncentiveActivity institutionalDeliveryActivityAM = incentivesRepo.findIncentiveMasterByNameAndGroup("MH_MOTIVATE_INST_DEL", GroupName.MATERNAL_HEALTH.getDisplayName()); | ||
| IncentiveActivity institutionalDeliveryActivityCH = incentivesRepo.findIncentiveMasterByNameAndGroup("INST_DELIVERY_ESCORT", GroupName.ACTIVITY.getDisplayName()); | ||
| if (deliveryOutcome.getPlaceOfDelivery() != null) { | ||
| String placeOfDelivery = deliveryOutcome.getPlaceOfDelivery(); | ||
|
|
||
| if (placeOfDelivery != null && | ||
| (!placeOfDelivery.equalsIgnoreCase("home") || | ||
| !placeOfDelivery.equalsIgnoreCase("in transit") || | ||
| !placeOfDelivery.equalsIgnoreCase("other private hospital"))) { | ||
|
|
||
| // Institutional delivery (eligible case) | ||
| if (institutionalDeliveryActivityAM != null) { | ||
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityAM); | ||
| if (institutionalDeliveryActivityCH != null) { | ||
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityCH); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Place-of-delivery condition is always true (|| of negations).
(!eq("home") || !eq("in transit") || !eq("other private hospital")) evaluates to true for every value — e.g. for "home", !eq("in transit") is already true. As written, the institutional-delivery incentive is granted for home/in-transit/private deliveries too, defeating the exclusion. Use &&.
🐛 Fix
- if (placeOfDelivery != null &&
- (!placeOfDelivery.equalsIgnoreCase("home") ||
- !placeOfDelivery.equalsIgnoreCase("in transit") ||
- !placeOfDelivery.equalsIgnoreCase("other private hospital"))) {
+ if (placeOfDelivery != null &&
+ !placeOfDelivery.equalsIgnoreCase("home") &&
+ !placeOfDelivery.equalsIgnoreCase("in transit") &&
+ !placeOfDelivery.equalsIgnoreCase("other private hospital")) {📝 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.
Suggested change
| if (placeOfDelivery != null && | |
| (!placeOfDelivery.equalsIgnoreCase("home") || | |
| !placeOfDelivery.equalsIgnoreCase("in transit") || | |
| !placeOfDelivery.equalsIgnoreCase("other private hospital"))) { | |
| // Institutional delivery (eligible case) | |
| if (institutionalDeliveryActivityAM != null) { | |
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityAM); | |
| } | |
| delOutList.forEach(deliveryOutcome -> { | |
| IncentiveActivity institutionalDeliveryActivityAM = incentivesRepo.findIncentiveMasterByNameAndGroup("MH_MOTIVATE_INST_DEL", GroupName.MATERNAL_HEALTH.getDisplayName()); | |
| IncentiveActivity institutionalDeliveryActivityCH = incentivesRepo.findIncentiveMasterByNameAndGroup("INST_DELIVERY_ESCORT", GroupName.ACTIVITY.getDisplayName()); | |
| if (deliveryOutcome.getPlaceOfDelivery() != null) { | |
| String placeOfDelivery = deliveryOutcome.getPlaceOfDelivery(); | |
| if (placeOfDelivery != null && | |
| (!placeOfDelivery.equalsIgnoreCase("home") || | |
| !placeOfDelivery.equalsIgnoreCase("in transit") || | |
| !placeOfDelivery.equalsIgnoreCase("other private hospital"))) { | |
| // Institutional delivery (eligible case) | |
| if (institutionalDeliveryActivityAM != null) { | |
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityAM); | |
| if (institutionalDeliveryActivityCH != null) { | |
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityCH); | |
| } | |
| } | |
| if (placeOfDelivery != null && | |
| !placeOfDelivery.equalsIgnoreCase("home") && | |
| !placeOfDelivery.equalsIgnoreCase("in transit") && | |
| !placeOfDelivery.equalsIgnoreCase("other private hospital")) { | |
| // Institutional delivery (eligible case) | |
| if (institutionalDeliveryActivityAM != null) { | |
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityAM); | |
| } | |
| if (institutionalDeliveryActivityCH != null) { | |
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityCH); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DeliveryOutcomeServiceImpl.java`
around lines 144 - 157, The place-of-delivery check in
DeliveryOutcomeServiceImpl is effectively always true because the negated
comparisons are joined with OR, so the institutional-delivery incentive is being
created even for excluded delivery locations. Update the condition in the
delivery processing block to require all excluded values to fail (use AND across
the negated equalsIgnoreCase checks), and keep the incentive creation calls in
the same conditional path so only valid institutional deliveries reach
createIncentiveRecordforJsy.
|
|
||
| if (institutionalDeliveryActivityCH != null) { | ||
| createIncentiveRecordforJsy(deliveryOutcome, deliveryOutcome.getBenId(), institutionalDeliveryActivityCH); | ||
| if (deliveryOutcome.getIsJSYBenificiary()) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
getIsJSYBenificiary() unboxed without null guard.
If isJSYBenificiary is null on any outcome, if (deliveryOutcome.getIsJSYBenificiary()) throws an NPE, failing the whole save with only a generic message. Use Boolean.TRUE.equals(...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/DeliveryOutcomeServiceImpl.java` at
line 160, The conditional in DeliveryOutcomeServiceImpl uses
getIsJSYBenificiary() directly, which can throw a NullPointerException when the
Boolean is null. Update the check in the save flow to use a null-safe Boolean
comparison such as Boolean.TRUE.equals(...) around
deliveryOutcome.getIsJSYBenificiary(), so the JSY branch only runs when the flag
is explicitly true.
Comment on lines
+244
to
+246
| if (entities.isEmpty()) { | ||
| return new Gson().toJson(Collections.emptyList()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Early return inside the page loop can discard already-accumulated results.
return new Gson().toJson(Collections.emptyList()) fires whenever a fetched page's content is empty. On the first page (no records at all) this is correct, but placing it inside the do/while means any empty page silently throws away everything already added to finalDtos. Prefer break (returning finalDtos) so accumulated pages are preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java` around
lines 244 - 246, The page-processing loop in IncentiveServiceImpl is returning
an empty JSON array too early when a fetched page is empty, which can discard
items already collected in finalDtos. Update the empty-page handling in the loop
so it stops pagination with a break or equivalent and then returns the
accumulated finalDtos JSON, using the existing entities/finalDtos flow in the
method instead of immediately returning Collections.emptyList().
Comment on lines
+295
to
299
| if (entry.getVerifiedByUserId() != null) { | ||
| entry.setSupervisorRole(userRepo.getUserRole(entry.getVerifiedByUserId()).get(0).getRoleName()); | ||
| entry.setVerifiedByUserName(userRepo.getUserRole(entry.getVerifiedByUserId()).get(0).getName()); | ||
|
|
||
| if (entry.getUpdatedBy() == null) { | ||
| entry.setUpdatedBy(userName); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Redundant DB call + unguarded .get(0) can throw.
userRepo.getUserRole(entry.getVerifiedByUserId()) is invoked twice for the same id, and .get(0) is called on the result without checking for an empty list. If the verifier has no role rows this throws IndexOutOfBoundsException, which (since this runs inside the paged mapping loop) aborts the whole listing. Resolve once and guard.
🛡️ Suggested fix
- if (entry.getVerifiedByUserId() != null) {
- entry.setSupervisorRole(userRepo.getUserRole(entry.getVerifiedByUserId()).get(0).getRoleName());
- entry.setVerifiedByUserName(userRepo.getUserRole(entry.getVerifiedByUserId()).get(0).getName());
-
- }
+ if (entry.getVerifiedByUserId() != null) {
+ var roles = userRepo.getUserRole(entry.getVerifiedByUserId());
+ if (roles != null && !roles.isEmpty()) {
+ entry.setSupervisorRole(roles.get(0).getRoleName());
+ entry.setVerifiedByUserName(roles.get(0).getName());
+ }
+ }📝 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.
Suggested change
| if (entry.getVerifiedByUserId() != null) { | |
| entry.setSupervisorRole(userRepo.getUserRole(entry.getVerifiedByUserId()).get(0).getRoleName()); | |
| entry.setVerifiedByUserName(userRepo.getUserRole(entry.getVerifiedByUserId()).get(0).getName()); | |
| if (entry.getUpdatedBy() == null) { | |
| entry.setUpdatedBy(userName); | |
| } | |
| if (entry.getVerifiedByUserId() != null) { | |
| var roles = userRepo.getUserRole(entry.getVerifiedByUserId()); | |
| if (roles != null && !roles.isEmpty()) { | |
| entry.setSupervisorRole(roles.get(0).getRoleName()); | |
| entry.setVerifiedByUserName(roles.get(0).getName()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java` around
lines 295 - 299, The verified-user enrichment in IncentiveServiceImpl is making
the same userRepo.getUserRole(...) call twice and using .get(0) without checking
for results, which can fail the paged mapping loop. Update the
entry.getVerifiedByUserId() branch to fetch the role list once, reuse it for
both supervisorRole and verifiedByUserName, and guard against a null/empty
result before reading the first element (or skip setting those fields when no
role rows exist).
| if (paiucdActivityAM != null && ancVisit.getIsPaiucdId() != null | ||
| && ancVisit.getIsPaiucdId().toString().equals("1")) { | ||
| recordAncRelatedIncentive(paiucdActivityAM, ancVisit); | ||
| if(ancVisit.getIsAborted()){ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
getIsAborted() unboxed without null guard (State 5 & State 8).
if(ancVisit.getIsAborted()) (Line 782, and again Line 839) unboxes a Boolean that elsewhere in this method is null-checked (e.g. Line 806 getIsAborted() != null). A null value here throws an NPE. Align with the guarded pattern used at Line 806.
🛡️ Suggested guard
- if(ancVisit.getIsAborted()){
+ if(Boolean.TRUE.equals(ancVisit.getIsAborted())){📝 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.
Suggested change
| if(ancVisit.getIsAborted()){ | |
| if(Boolean.TRUE.equals(ancVisit.getIsAborted())){ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/flw/service/impl/MaternalHealthServiceImpl.java` at
line 782, The `MaternalHealthServiceImpl` logic is dereferencing
`ancVisit.getIsAborted()` as a primitive condition without a null check, unlike
the guarded usage already present later in the same method. Update the `if`
checks in the `ancVisit` handling flow so they first verify `getIsAborted()` is
not null before evaluating its boolean value, and apply the same fix to both
occurrences of this pattern in the method to keep the behavior consistent and
avoid NPEs.
optmiz query for incentive logic for hbnc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
📋 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
Bug Fixes
Performance / Usability