18615 daily returns downloaded excel is not matching the display#18621
Conversation
…e-display' of https://github.com/hmislk/hmis.git into 18615-daily-returns-downloaded-excel-is-not-matching-the-display
WalkthroughThis change refactors ExcelController to introduce specialized rendering methods for various bundle types and payment methods, replacing generic handlers. It adds dedicated exporters for DTO-based bundles with enhanced dispatch routing to direct different data types to their appropriate processors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.5)src/main/java/com/divudi/bean/common/ExcelController.javaTip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbcf3ece1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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 (1)
src/main/java/com/divudi/bean/common/ExcelController.java (1)
3513-3515:⚠️ Potential issue | 🟡 MinorNo-arg overload defaults to
new Date()for both from/to — callers likely expect search dates.
createExcelForDtoBundle(rootBundle)passesnew Date(), new Date()as the date range. UnlikecreateExcelForBundle(line 52-54) which reads fromsearchController.getFromDate()/getToDate(), this overload hardcodes "now" for both bounds. If the caller intended to use the search range, this will produce incorrect header text and filename.Suggested fix: delegate to search dates like the non-DTO overload
public StreamedContent createExcelForDtoBundle(com.divudi.core.data.dto.DailyReturnBundleDTO rootBundle) throws IOException { - return createExcelForDtoBundle(rootBundle, new Date(), new Date()); + return createExcelForDtoBundle(rootBundle, searchController.getFromDate(), searchController.getToDate()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/divudi/bean/common/ExcelController.java` around lines 3513 - 3515, The no-arg overload createExcelForDtoBundle(DailyReturnBundleDTO) incorrectly uses new Date() for both from/to; change it to delegate to the searchController's dates like the non-DTO overload: call createExcelForDtoBundle(rootBundle, searchController.getFromDate(), searchController.getToDate()) so header text/filename use the current search range (ensure searchController is accessible in ExcelController).
🧹 Nitpick comments (2)
src/main/java/com/divudi/bean/common/ExcelController.java (2)
2054-2080: Consider extracting shared rendering patterns to reduce duplication.The new methods (
addDataToExcelForChequePayments,addDataToExcelForEwalletPayments,addDataToExcelForSlipPayments,addDataToExcelForPatientDepositUtilization,addDataToExcelForOpdPatientDepositPayments,addDataToExcelForInwardPatientDepositPayments) follow a nearly identical structure: title row → header row → data rows → optional footer. They differ only in column definitions and how each column maps to the data model.A table-driven approach (e.g., a column-descriptor list with a single generic rendering method) would dramatically reduce this ~500+ lines of boilerplate while making future column changes safer and easier.
This is not blocking, but worth tracking for a follow-up.
Would you like me to open a separate issue to track the refactoring of these Excel rendering methods into a table-driven approach?
Also applies to: 2376-2583, 2782-3001
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/divudi/bean/common/ExcelController.java` around lines 2054 - 2080, Several Excel-rendering methods (e.g., addDataToExcelForChequePayments, addDataToExcelForEwalletPayments, addDataToExcelForSlipPayments, addDataToExcelForPatientDepositUtilization, addDataToExcelForOpdPatientDepositPayments, addDataToExcelForInwardPatientDepositPayments and addDataToExcelForCollectionForTheDay) duplicate the same pattern (create styles, write title/header, iterate rows, write footer); extract a single generic renderer that accepts a sheet, startRow, a title, a List/array of ColumnDescriptor objects (each descriptor defines header label, value extractor lambda/method reference, and optional format/style) and the data list, then implement each specific method by building the descriptor list and calling that generic method; ensure the generic method reuses CellStyle creation (e.g., boldStyle, numberStyle, boldNumberStyle) and supports optional footer aggregation so existing callers (those methods named above) can be replaced with a one-liner that supplies column mappings and footer logic.
3517-3578:XSSFWorkbookis not protected by try-with-resources — resource leak on exception.If an exception is thrown between
new XSSFWorkbook()(line 3523) andworkbook.close()(line 3564), the workbook's underlying resources are leaked. Wrap the workbook in a try-with-resources block.Proposed fix
public StreamedContent createExcelForDtoBundle(com.divudi.core.data.dto.DailyReturnBundleDTO rootBundle, Date fromDate, Date toDate) throws IOException { if (rootBundle == null) { return null; } - StreamedContent excelSc; - - XSSFWorkbook workbook = new XSSFWorkbook(); + try (XSSFWorkbook workbook = new XSSFWorkbook()) { XSSFSheet dataSheet = workbook.createSheet(rootBundle.getName()); // ... existing body ... - workbook.write(outputStream); - workbook.close(); + workbook.write(outputStream); + } // workbook auto-closed // ... build StreamedContent ...Note: the same issue exists in
createExcelForBundle(line 61), but since that method is not changed in this PR, it can be addressed separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/divudi/bean/common/ExcelController.java` around lines 3517 - 3578, The XSSFWorkbook in createExcelForDtoBundle is not guarded against exceptions and can leak resources; refactor to use try-with-resources for the XSSFWorkbook (and the ByteArrayOutputStream) so workbook.close() is guaranteed even on exceptions, perform all sheet/row/cell operations and workbook.write(outputStream) inside that try block, then create the byte[]/ByteArrayInputStream after the try so the workbook and outputStream are closed; keep usage of addDtoDataToExcel and the existing stream() builder but ensure the InputStream is created from the final byte[] (not the workbook) so no workbook resource remains open.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/divudi/bean/common/ExcelController.java`:
- Around line 2838-2844: The code creates a new CellStyle inside each data-row
loop which wastes workbook styles; for each method named
addDataToExcelForPatientDepositUtilization,
addDataToExcelForOpdPatientDepositPayments, and
addDataToExcelForInwardPatientDepositPayments, move creation of the date
CellStyle (the code that calls dataSheet.getWorkbook().createCellStyle() and
sets the data format "yyyy-MM-dd HH:mm") out of the per-row loop and create it
once (similar to the existing numberStyle), then reuse that single dateStyle for
dateCell.setCellStyle(...) inside the loop.
---
Outside diff comments:
In `@src/main/java/com/divudi/bean/common/ExcelController.java`:
- Around line 3513-3515: The no-arg overload
createExcelForDtoBundle(DailyReturnBundleDTO) incorrectly uses new Date() for
both from/to; change it to delegate to the searchController's dates like the
non-DTO overload: call createExcelForDtoBundle(rootBundle,
searchController.getFromDate(), searchController.getToDate()) so header
text/filename use the current search range (ensure searchController is
accessible in ExcelController).
---
Nitpick comments:
In `@src/main/java/com/divudi/bean/common/ExcelController.java`:
- Around line 2054-2080: Several Excel-rendering methods (e.g.,
addDataToExcelForChequePayments, addDataToExcelForEwalletPayments,
addDataToExcelForSlipPayments, addDataToExcelForPatientDepositUtilization,
addDataToExcelForOpdPatientDepositPayments,
addDataToExcelForInwardPatientDepositPayments and
addDataToExcelForCollectionForTheDay) duplicate the same pattern (create styles,
write title/header, iterate rows, write footer); extract a single generic
renderer that accepts a sheet, startRow, a title, a List/array of
ColumnDescriptor objects (each descriptor defines header label, value extractor
lambda/method reference, and optional format/style) and the data list, then
implement each specific method by building the descriptor list and calling that
generic method; ensure the generic method reuses CellStyle creation (e.g.,
boldStyle, numberStyle, boldNumberStyle) and supports optional footer
aggregation so existing callers (those methods named above) can be replaced with
a one-liner that supplies column mappings and footer logic.
- Around line 3517-3578: The XSSFWorkbook in createExcelForDtoBundle is not
guarded against exceptions and can leak resources; refactor to use
try-with-resources for the XSSFWorkbook (and the ByteArrayOutputStream) so
workbook.close() is guaranteed even on exceptions, perform all sheet/row/cell
operations and workbook.write(outputStream) inside that try block, then create
the byte[]/ByteArrayInputStream after the try so the workbook and outputStream
are closed; keep usage of addDtoDataToExcel and the existing stream() builder
but ensure the InputStream is created from the final byte[] (not the workbook)
so no workbook resource remains open.
buddhika75
deleted the
18615-daily-returns-downloaded-excel-is-not-matching-the-display
branch
1 participant
Summary by CodeRabbit
Release Notes