SSEMR-471 Match the total TX Curr on graph, card and regimens #54
Conversation
WalkthroughThis pull request updates the patient summary generation functionality. The method in the summary generator now accepts an additional parameter to enforce that the last month's patient count matches the provided total. Corresponding method calls in the Changes
Sequence Diagram(s)sequenceDiagram
participant TxCurr as TxCurrController
participant Generator as GenerateCumulativeSummary
participant Logger as System Logger
TxCurr->>Generator: generateCumulativeSummary(dates, startDate, endDate, totalPatients)
Generator->>Generator: Compute monthly summary
Generator->>Generator: Validate last month count equals totalPatients
alt Count mismatch detected
Generator->>Logger: Log warning about count adjustment
end
Generator-->>TxCurr: Return summary map
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
omod/src/main/java/org/openmrs/module/ssemrws/web/constants/GenerateCumulativeSummary.java (2)
40-47: Consider adding validation for the totalPatients parameter.While the logic to enforce the total patients count is correct, consider adding validation to ensure
totalPatientsis non-negative and at least equal to the count from the previous month to maintain data consistency.// Step 3: Ensure total patients count is correct in the last month Calendar endCal = Calendar.getInstance(); endCal.setTime(endDate); String lastMonth = months[endCal.get(Calendar.MONTH)]; +// Validate totalPatients parameter +if (totalPatients < 0) { + throw new IllegalArgumentException("Total patients count cannot be negative"); +} + +String previousMonth = months[(endCal.get(Calendar.MONTH) + 11) % 12]; +if (cumulativeSummary.containsKey(previousMonth) && + totalPatients < cumulativeSummary.get(previousMonth)) { + throw new IllegalArgumentException("Total patients count cannot be less than previous month's count"); +} + // Force last month count to be exactly totalPatients cumulativeSummary.put(lastMonth, totalPatients);
63-67: Consider using a logging framework instead of System.err.While the warning message is helpful, using a proper logging framework would be more appropriate for production code. This would provide better control over log levels and output destinations.
-System.err.println("Warning: Adjusting final month count to match total patients."); +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +private static final Log log = LogFactory.getLog(GenerateCumulativeSummary.class); +log.warn("Adjusting final month count to match total patients.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
omod/src/main/java/org/openmrs/module/ssemrws/web/constants/GenerateCumulativeSummary.java(3 hunks)omod/src/main/java/org/openmrs/module/ssemrws/web/controller/SSEMRWebServicesController.java(2 hunks)omod/src/main/java/org/openmrs/module/ssemrws/web/controller/TxCurrController.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- omod/src/main/java/org/openmrs/module/ssemrws/web/controller/SSEMRWebServicesController.java
🔇 Additional comments (4)
omod/src/main/java/org/openmrs/module/ssemrws/web/constants/GenerateCumulativeSummary.java (3)
8-9: LGTM! Method signature updated to support total patient count validation.The addition of the
totalPatientsparameter aligns with the PR objective to match totals across different views.
48-51: LGTM! Step renumbering improves code organization.The filtering logic is well-placed and properly documented.
68-68: LGTM! Clear documentation of final step.The comment clearly indicates the purpose of the final map creation.
omod/src/main/java/org/openmrs/module/ssemrws/web/controller/TxCurrController.java (1)
71-72: LGTM! Improved formatting and consistency.The changes improve code readability and ensure consistency in patient counts by passing the
totalCountparameter toGenerateCumulativeSummary.generateCumulativeSummary.Please verify that:
- The
totalCountmatches the actual number of patients in the list- The
GenerateCumulativeSummary.generateCumulativeSummarymethod properly handles the new parameter❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for the implementation of generateCumulativeSummary to verify parameter handling ast-grep --pattern 'class GenerateCumulativeSummary { $$$ generateCumulativeSummary($$$) { $$$ } $$$ }'Length of output: 117
Action Required: Manual Verification Needed for GenerateCumulativeSummary Parameter Handling
The automated search didn't yield evidence of the updated implementation of the
generateCumulativeSummarymethod. Please manually verify the following:
- That the
totalCountparameter correctly reflects the actual number of patients in the list.- That the
GenerateCumulativeSummary.generateCumulativeSummarymethod is updated to properly accept and use the newtotalCountparameter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
omod/src/main/java/org/openmrs/module/ssemrws/web/constants/GetPatientRegimens.java (1)
110-128: Consider thread safety and modern date-time API improvements.The implementation has several areas for improvement:
SimpleDateFormatis not thread-safe, which could cause issues in a multi-threaded environment.- The legacy
DateandCalendarAPIs are used instead of the more robust modernjava.timeAPI.- No input validation for malformed date strings.
Consider refactoring to use the modern
java.timeAPI:-public static Date[] getStartAndEndDate(String qStartDate, String qEndDate, SimpleDateFormat dateTimeFormatter) - throws ParseException { - Date endDate = (qEndDate != null) ? dateTimeFormatter.parse(qEndDate) : new Date(); - - // Extend endDate to 23:59:59 - Calendar calendar = Calendar.getInstance(); - calendar.setTime(endDate); - calendar.set(Calendar.HOUR_OF_DAY, 23); - calendar.set(Calendar.MINUTE, 59); - calendar.set(Calendar.SECOND, 59); - calendar.set(Calendar.MILLISECOND, 999); - endDate = calendar.getTime(); - - // Set startDate correctly - calendar.set(Calendar.DAY_OF_MONTH, 1); - Date startDate = (qStartDate != null) ? dateTimeFormatter.parse(qStartDate) : calendar.getTime(); - - return new Date[] { startDate, endDate }; +public static Date[] getStartAndEndDate(String qStartDate, String qEndDate, DateTimeFormatter formatter) { + LocalDate endLocalDate = qEndDate != null ? + LocalDate.parse(qEndDate, formatter) : + LocalDate.now(); + + // Convert to Date with end of day time + Date endDate = Date.from(endLocalDate + .atTime(23, 59, 59, 999_000_000) + .atZone(ZoneId.systemDefault()) + .toInstant()); + + // Set startDate to first of month if not provided + LocalDate startLocalDate = qStartDate != null ? + LocalDate.parse(qStartDate, formatter) : + endLocalDate.withDayOfMonth(1); + + Date startDate = Date.from(startLocalDate + .atStartOfDay() + .atZone(ZoneId.systemDefault()) + .toInstant()); + + return new Date[] { startDate, endDate }; +}Also add input validation:
if (qStartDate != null && !qStartDate.matches("\\d{4}-\\d{2}-\\d{2}")) { throw new IllegalArgumentException("Invalid start date format. Expected: yyyy-MM-dd"); } if (qEndDate != null && !qEndDate.matches("\\d{4}-\\d{2}-\\d{2}")) { throw new IllegalArgumentException("Invalid end date format. Expected: yyyy-MM-dd"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
omod/src/main/java/org/openmrs/module/ssemrws/web/constants/GetPatientRegimens.java(1 hunks)omod/src/main/java/org/openmrs/module/ssemrws/web/controller/SSEMRWebServicesController.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- omod/src/main/java/org/openmrs/module/ssemrws/web/controller/SSEMRWebServicesController.java
🔇 Additional comments (1)
omod/src/main/java/org/openmrs/module/ssemrws/web/constants/GetPatientRegimens.java (1)
22-23: LGTM! Good integration of date handling.The new method is well integrated into the existing code and provides consistent date range handling across the application.
Summary by CodeRabbit
New Features
Style
Refactor