Fix the count issue in HRP #106
Conversation
WalkthroughVersion bumped to 3.5.0 with SLF4J dependencies added. Role-related fields and Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
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/ecd/repo/call_conf_allocation/OutboundCallsRepo.java (1)
125-131: Critical: Missing required parameters in method signature.The query at line 126 references
:fDateand:tDateparameters, but the method signature at lines 130-131 does not declare these parameters. This will cause a runtime exception when the query is executed.Apply this diff to add the missing parameters:
int getMotherUnAllocatedCountHR(@Param("allocationStatus") String allocationStatus, @Param("psmId") Integer psmId, - @Param("phoneNoType") String phoneNoType); + @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("phoneNoType") String phoneNoType);src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (1)
557-557: Missing date range parameters in Mother high-risk count query.Line 557 calls
getMotherUnAllocatedCountHRwithout date range parameters (tempFDateStamp,tempTDateStamp), while the low-risk query on line 555 includes them. This inconsistency means the high-risk count may include records outside the user's specified date range, which is likely the "count issue in HRP" mentioned in the PR title.Compare to line 555:
totalLowRisk = outboundCallsRepo.getMotherUnAllocatedCountLR(Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType);And line 568 for child records which correctly includes date parameters:
totalHighRisk = outboundCallsRepo.getChildUnAllocatedCountHR("unallocated", psmId, tempFDateStamp, tempTDateStamp, phoneNoType);Apply this diff to add the missing date range parameters:
- totalHighRisk = outboundCallsRepo.getMotherUnAllocatedCountHR(Constants.UNALLOCATED, psmId,phoneNoType); + totalHighRisk = outboundCallsRepo.getMotherUnAllocatedCountHR(Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType);Note: This assumes the repository method signature has been updated to accept date parameters. If not, the repository method also needs to be updated.
src/main/java/com/iemr/ecd/service/quality/QualityAuditImpl.java (1)
149-156: Date rollover and millisecond bugs produce wrong ranges (affects counts).
- If minusMonth==1 and month==January, year isn’t decremented (uses “-0”), yielding previous-month math against same year.
- Milliseconds set to 59 instead of 999, truncating end-of-day by ~940 ms.
Fix both to ensure accurate SP date windows.
@@ - if(qualityAuditorWorklistRequestDTO.getMonth() == 1) { - call.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear() -0); - }else { - call.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear()); - } - call.set(Calendar.MONTH, qualityAuditorWorklistRequestDTO.getMonth() - minusMonth - 1); + if (minusMonth == 1 && qualityAuditorWorklistRequestDTO.getMonth() == 1) { + call.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear() - 1); + } else { + call.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear()); + } + call.set(Calendar.MONTH, qualityAuditorWorklistRequestDTO.getMonth() - minusMonth - 1); @@ - if(qualityAuditorWorklistRequestDTO.getMonth() ==1) { - cal2.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear() -0); - }else { - cal2.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear()); - } - cal2.set(Calendar.MONTH, qualityAuditorWorklistRequestDTO.getMonth() - minusMonth -1); + if (minusMonth == 1 && qualityAuditorWorklistRequestDTO.getMonth() == 1) { + cal2.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear() - 1); + } else { + cal2.set(Calendar.YEAR, qualityAuditorWorklistRequestDTO.getYear()); + } + cal2.set(Calendar.MONTH, qualityAuditorWorklistRequestDTO.getMonth() - minusMonth - 1); @@ - cal2.set(Calendar.MILLISECOND, 59); + cal2.set(Calendar.MILLISECOND, 999); @@ - cal2.set(Calendar.MILLISECOND, 59); + cal2.set(Calendar.MILLISECOND, 999);Also applies to: 174-185, 221-226
src/main/java/com/iemr/ecd/service/quality/QualityAuditQuestionConfigurationImpl.java (1)
58-88: Critical: optionsSet leaks across loop iterations, duplicating options onto subsequent questions.optionsSet is declared outside the loop and not reset when a question has no options, causing previous options to be re-saved under the next question id.
- QualityAuditQuestionnaireValues questionnaireOptions; - Set<QualityAuditQuestionnaireValues> optionsSet = null; + QualityAuditQuestionnaireValues questionnaireOptions; @@ - for (QualityAuditQuestionConfig question : qualityAuditQuestionConfig) { + for (QualityAuditQuestionConfig question : qualityAuditQuestionConfig) { + Set<QualityAuditQuestionnaireValues> optionsSet = null; // reset per question if (question.getRoles() != null && !question.getRoles().isEmpty()) { question.flattenRoles(); } if (question.getOptions() != null && question.getOptions().length > 0) { int j = 0; Integer[] scoreArr = question.getScores(); optionsSet = new HashSet<>(); for (String option : question.getOptions()) { questionnaireOptions = new QualityAuditQuestionnaireValues(); questionnaireOptions.setQuestionValues(option); questionnaireOptions.setScore(scoreArr[j]); questionnaireOptions.setPsmId(question.getPsmId()); questionnaireOptions.setCreatedBy(question.getCreatedBy()); optionsSet.add(questionnaireOptions); j++; } } QualityAuditQuestionConfig resultSet = qualityAuditQuestionConfigRepo.save(question); if (resultSet != null && resultSet.getId() != null && optionsSet != null && optionsSet.size() > 0) { for (QualityAuditQuestionnaireValues optionsValues : optionsSet) { optionsValues.setQuestionId(resultSet.getId()); } qualityAuditQuestionnaireValuesRepo.saveAll(optionsSet); } }
🧹 Nitpick comments (9)
pom.xml (1)
41-46: Clarify the commented-out logging exclusions.The exclusions for
spring-boot-starter-loggingare now commented out, which means the default Logback logging will be active. If this is intentional to restore default logging behavior, consider removing the commented block entirely for cleaner code. If the exclusions might be needed in the future, document why they're commented out.src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (1)
570-571: Inconsistent debug logging between Mother and Child branches.Debug logging is added only for the Child record path but not for the Mother record path (which has the date range issue on line 557). For consistent debugging and troubleshooting, consider adding similar logging to the Mother branch at line 558.
Add logging for Mother branch as well:
totalHighRisk = outboundCallsRepo.getMotherUnAllocatedCountHR(Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType); + logger.info("Params="+psmId+","+tempFDateStamp+","+tempTDateStamp+","+phoneNoType); + logger.info("High risk count: "+totalHighRisk); totalAllocated = outboundCallsRepo.getTotalAllocatedCountMother(Constants.ALLOCATED, psmId, tempFDateStamp,Additionally, consider using parameterized logging for better performance:
logger.info("Params={},{},{},{}", psmId, tempFDateStamp, tempTDateStamp, phoneNoType); logger.info("High risk count: {}", totalHighRisk);src/main/java/com/iemr/ecd/dao/V_get_Qualityaudit_SectionQuestionaireValues.java (1)
79-81: Role mapping looks fine; mark read-only if this maps a DB view.Given this maps V_get_Qualityaudit_Sectionquestionairevalues, make Role non-insertable/updatable for safety.
- @Column(name = "Role") + @Column(name = "Role", insertable = false, updatable = false) private String role;src/main/java/com/iemr/ecd/dao/QualityAuditQuestionConfig.java (1)
92-103: Role fields + flattenRoles() — OK; confirm DB column size.Join-only approach is fine here. Consider constraining column length to match DB.
- @Column(name = "Role") + @Column(name = "Role", length = 255) private String role;Based on learnings.
src/main/java/com/iemr/ecd/dto/ResponseCallAuditSectionQuestionMapDTO.java (1)
51-51: LGTM; optionally omit empty roles from JSON.+import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonInclude.Include; @@ -@Data +@Data +@JsonInclude(Include.NON_EMPTY) public class ResponseCallAuditSectionQuestionMapDTO {src/main/java/com/iemr/ecd/dto/QualityAuditorSectionQuestionaireResponseDTO.java (1)
57-57: LGTM; optionally omit empty roles from JSON.+import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonInclude.Include; @@ -@Data +@Data +@JsonInclude(Include.NON_EMPTY) public class QualityAuditorSectionQuestionaireResponseDTO {src/main/java/com/iemr/ecd/service/quality/QualityAuditImpl.java (1)
369-371: Trim and ignore empty tokens when splitting roles.Prevents stray spaces/empties from DB strings.
+import java.util.stream.Collectors; @@ - if(obj.getRole() != null && !obj.getRole().isEmpty()) { - responseDTO.setRoles(Arrays.asList(obj.getRole().split(","))); - } + if (obj.getRole() != null && !obj.getRole().isEmpty()) { + responseDTO.setRoles( + Arrays.stream(obj.getRole().split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toList())); + }src/main/java/com/iemr/ecd/service/quality/QualityAuditQuestionConfigurationImpl.java (2)
144-146: Parsing roles — consider trimming to avoid stray spaces.- obj.setRoles(Arrays.asList(strArr[14].split(","))); + obj.setRoles(Arrays.stream(strArr[14].split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .toList());
90-93: Minor: “Qulaity” typo in responses.-responseMap.put("response", "Qulaity Audit Questionnaire Created Successfully"); +responseMap.put("response", "Quality Audit Questionnaire Created Successfully"); @@ -responseMap.put("response", "Qulaity Audit Questionnaire Updated Successfully"); +responseMap.put("response", "Quality Audit Questionnaire Updated Successfully");Also applies to: 211-214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pom.xml(2 hunks)src/main/java/com/iemr/ecd/dao/QualityAuditQuestionConfig.java(2 hunks)src/main/java/com/iemr/ecd/dao/V_get_Qualityaudit_SectionQuestionaireValues.java(1 hunks)src/main/java/com/iemr/ecd/dto/QualityAuditorSectionQuestionaireResponseDTO.java(1 hunks)src/main/java/com/iemr/ecd/dto/ResponseCallAuditSectionQuestionMapDTO.java(1 hunks)src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java(1 hunks)src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java(3 hunks)src/main/java/com/iemr/ecd/service/quality/QualityAuditImpl.java(1 hunks)src/main/java/com/iemr/ecd/service/quality/QualityAuditQuestionConfigurationImpl.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T03:21:21.540Z
Learnt from: 5Amogh
PR: PSMRI/ECD-API#104
File: src/main/java/com/iemr/ecd/dao/QualityAuditQuestionConfig.java:92-103
Timestamp: 2025-08-12T03:21:21.540Z
Learning: In the ECD-API codebase, roles in QualityAuditQuestionConfig are sourced from the database in a controlled manner without user input, so extensive normalization (trimming, filtering nulls/empties) is unnecessary overhead in the flattenRoles() method.
Applied to files:
src/main/java/com/iemr/ecd/service/quality/QualityAuditQuestionConfigurationImpl.javasrc/main/java/com/iemr/ecd/service/quality/QualityAuditImpl.javasrc/main/java/com/iemr/ecd/dao/QualityAuditQuestionConfig.java
🔇 Additional comments (2)
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (1)
56-57: LGTM!The SLF4J logger setup is correct and follows best practices. However, ensure the dependency issues in
pom.xmlare resolved first for this to work properly.Also applies to: 72-72
src/main/java/com/iemr/ecd/service/quality/QualityAuditQuestionConfigurationImpl.java (1)
62-65: Good call to flatten roles before save.
| <dependency> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-api</artifactId> | ||
| <version>${slf4j.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-simple</artifactId> | ||
| <version>${slf4j.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
Potential logging framework conflict.
Adding slf4j-simple alongside Spring Boot's default logging setup (Logback via spring-boot-starter-logging) can cause conflicts. Spring Boot already provides SLF4J API and Logback as the logging implementation. The slf4j-simple binding will compete with Logback, potentially leading to unpredictable logging behavior or startup warnings.
Additionally, the ${slf4j.version} property is not defined in the <properties> section (lines 18-29), which will cause a build failure.
Apply this diff to remove the conflicting slf4j-simple dependency and add the missing property:
<properties>
<environment>${ENV_VAR}</environment>
<target-properties>target/classes/application.properties</target-properties>
<source-properties>target/classes/ecd_${environment}.properties</source-properties>
<java.version>17</java.version>
+ <slf4j.version>2.0.9</slf4j.version>
<jacoco.version>0.8.11</jacoco.version>- </dependency>
- <dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-api</artifactId>
- <version>${slf4j.version}</version>
- </dependency>
- <dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-simple</artifactId>
- <version>${slf4j.version}</version>
</dependency>Note: Spring Boot Starter already transitively includes slf4j-api, so explicit declaration is unnecessary.
📝 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.
| <dependency> | |
| <groupId>org.slf4j</groupId> | |
| <artifactId>slf4j-api</artifactId> | |
| <version>${slf4j.version}</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.slf4j</groupId> | |
| <artifactId>slf4j-simple</artifactId> | |
| <version>${slf4j.version}</version> | |
| </dependency> | |
| <properties> | |
| <environment>${ENV_VAR}</environment> | |
| <target-properties>target/classes/application.properties</target-properties> | |
| <source-properties>target/classes/ecd_${environment}.properties</source-properties> | |
| <java.version>17</java.version> | |
| <slf4j.version>2.0.9</slf4j.version> | |
| <jacoco.version>0.8.11</jacoco.version> |
🤖 Prompt for AI Agents
In pom.xml around lines 48-57, remove the explicit org.slf4j:slf4j-simple
dependency (and the explicit org.slf4j:slf4j-api dependency since Spring Boot
provides SLF4J transitively) to avoid binding conflicts with Logback, and add
the missing <slf4j.version> property to the <properties> section (or remove any
references to that property if you drop the explicit SLF4J dependencies) so the
build doesn’t fail due to an undefined property.
|
Track in another PR in 3.9.0 |
📋 Description
JIRA ID:
AMM-1763
✅ Type of Change
Summary by CodeRabbit
Release Notes
New Features
Chores