Conversation
Updated the PDF generation. If issue, still persists, give more details. Signed-off-by: Dr M H B Ariyaratne <buddhika.ari@gmail.com>
…hmis.git into 9121-daily-return-report
WalkthroughAdded extensive PDF rendering logic to PdfController: new private helpers for multiple payment/deposit bundle types, expanded addDataToPdf switch routing, standardized date and numeric formatting, and a DTO-based PDF generation flow for DailyReturnBundleDTO including header, row, and total rendering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (1)
src/main/java/com/divudi/bean/common/PdfController.java (1)
738-860:⚠️ Potential issue | 🟠 MajorPre-existing bug: orphan empty table appended after every section.
Line 738 creates a 6-column
Tableand Line 856 unconditionally adds it to the document viadocument.add(table). This happens for every case, including the new ones at Lines 829–849. Each populate method already adds its own table(s) to the document, so this extra empty table is unintended noise in the PDF output.Consider moving the
tablecreation anddocument.add(table)into only thedefaultcase, or using areturn/ guard pattern so that the orphan table is not appended when a specific case handler has already written its content.🤖 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/PdfController.java` around lines 738 - 860, An empty 6-column Table is created at Table table = new Table(new float[]{10, 40, 15, 15, 10, 10}) and unconditionally added via document.add(table), causing an orphan empty table after every case (including handlers like populateTableForItemSummaryGroupedByCategory, populateTableForDepartmentCollection, populateTableForCompanyCollection, etc.). Fix by removing the unconditional table creation/addition: either move the Table creation and document.add(table) into the default case only (so only unknown types get that table) or add a guard/early-return after each case that writes its own tables so document.add(table) is only executed when no populate* method has already emitted content; ensure references to Table/table and document.add(table) are the only places changed.
🧹 Nitpick comments (2)
src/main/java/com/divudi/bean/common/PdfController.java (2)
2332-2401: Unusedtypeparameter — all DTO bundles render identically.The
typeparameter is accepted but never referenced in the method body. EveryDailyReturnBundleDTOwill render the same 7-column table regardless ofbundleType. If different bundle types need different layouts (as in the non-DTOaddDataToPdfswitch), this will need a switch/dispatch. If all DTO bundles intentionally share one layout, remove the parameter to avoid confusion.🤖 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/PdfController.java` around lines 2332 - 2401, The method addDtoDataToPdf currently accepts a String type parameter that is never used, causing confusion about differing bundle layouts; either remove the unused parameter from addDtoDataToPdf (and update all call sites) or implement dispatch logic similar to addDataToPdf to vary the table layout based on the type value for DailyReturnBundleDTO bundles, ensuring addDtoBundleTotal remains called; update signatures and callers accordingly.
2297-2330: Consider try-with-resources forDocument/PdfDocumentto guard against resource leaks.If an exception is thrown between
new Document(pdf)(Line 2306) anddocument.close()(Line 2316), the PDF writer won't be finalized. This mirrors the existingcreatePdfForBundlepattern, so it's not a regression, but wrapping in try-with-resources would be safer.🤖 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/PdfController.java` around lines 2297 - 2330, Wrap the PDF creation resources in a try-with-resources block to ensure PdfWriter, PdfDocument and Document are always closed: move creation of PdfWriter, PdfDocument and Document (currently in createPdfForDtoBundle) into a try(...) block (optionally include ByteArrayOutputStream there too), perform the addDtoDataToPdf calls inside that block, and only convert the outputStream to a byte[]/ByteArrayInputStream after the try block so resources are reliably closed even on exceptions.
🤖 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/PdfController.java`:
- Around line 829-849: The monetary formatting in
populateTableForopdServiceBilled and
populateTableForpharmacyServiceCancellations is using a DecimalFormat pattern
string with String.format (e.g. String.format("#,##0.00", ...)) which outputs
the literal pattern; change those calls to use a proper printf-style format like
String.format("%,.2f", <numericValue>) (or alternatively create and use a
DecimalFormat instance and call format(value)) for every monetary column in
those two methods so amounts for
PharmacyCreditBills/PharmacyCreditCancel/PharmacyCreditRefund routes render
correctly; search for all occurrences of String.format("#,##0.00", ...) in
populateTableForopdServiceBilled and
populateTableForpharmacyServiceCancellations and replace them consistently.
---
Outside diff comments:
In `@src/main/java/com/divudi/bean/common/PdfController.java`:
- Around line 738-860: An empty 6-column Table is created at Table table = new
Table(new float[]{10, 40, 15, 15, 10, 10}) and unconditionally added via
document.add(table), causing an orphan empty table after every case (including
handlers like populateTableForItemSummaryGroupedByCategory,
populateTableForDepartmentCollection, populateTableForCompanyCollection, etc.).
Fix by removing the unconditional table creation/addition: either move the Table
creation and document.add(table) into the default case only (so only unknown
types get that table) or add a guard/early-return after each case that writes
its own tables so document.add(table) is only executed when no populate* method
has already emitted content; ensure references to Table/table and
document.add(table) are the only places changed.
---
Nitpick comments:
In `@src/main/java/com/divudi/bean/common/PdfController.java`:
- Around line 2332-2401: The method addDtoDataToPdf currently accepts a String
type parameter that is never used, causing confusion about differing bundle
layouts; either remove the unused parameter from addDtoDataToPdf (and update all
call sites) or implement dispatch logic similar to addDataToPdf to vary the
table layout based on the type value for DailyReturnBundleDTO bundles, ensuring
addDtoBundleTotal remains called; update signatures and callers accordingly.
- Around line 2297-2330: Wrap the PDF creation resources in a try-with-resources
block to ensure PdfWriter, PdfDocument and Document are always closed: move
creation of PdfWriter, PdfDocument and Document (currently in
createPdfForDtoBundle) into a try(...) block (optionally include
ByteArrayOutputStream there too), perform the addDtoDataToPdf calls inside that
block, and only convert the outputStream to a byte[]/ByteArrayInputStream after
the try block so resources are reliably closed even on exceptions.
![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: 4a7ec58d7e
ℹ️ 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".
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 (3)
src/main/java/com/divudi/bean/common/PdfController.java (3)
738-739:⚠️ Potential issue | 🟠 MajorOrphan empty table added to every PDF page.
Lines 738–739 create a 6-column
tablethat is unconditionally added to the document at line 856, but everycasebranch creates and adds its own table internally. This results in an extra empty table appended after each real table in the PDF output. The new cases (lines 829–849) inherit this bug.The
defaultcase at line 851 is the only branch that writes into this orphan table, so the fix is to move the table creation anddocument.add(table)into thedefaultblock.Proposed fix
private void addDataToPdf(Document document, ReportTemplateRowBundle addingBundle, String type) { if (type == null || type.isEmpty()) { type = "BillList"; } - // Create a new Table for each call - Table table = new Table(new float[]{10, 40, 15, 15, 10, 10}); - table.setWidth(200); - switch (type) { // ... all cases ... default: + Table table = new Table(new float[]{10, 40, 15, 15, 10, 10}); + table.setWidth(200); table.addCell(new Cell().add(new Paragraph("Data for unknown type"))); + document.add(table); break; } - - // Add the table to the document - document.add(table); - - // Optionally, add spacing or a separator between tables - document.add(new Paragraph("\n")); }Also applies to: 850-859
🤖 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/PdfController.java` around lines 738 - 739, The orphan empty 6-column Table created by "Table table = new Table(new float[]{10, 40, 15, 15, 10, 10});" and the unconditional "document.add(table)" are always appended after each branch; move the creation and the document.add(table) call into the default branch so only the default case uses that table. Update the PdfController method where the switch/case creates per-branch tables (the new cases at lines ~829–849) to remove the shared table variable and remove the unconditional document.add(table) at the end; instead instantiate the table and call document.add(table) inside the default case only.
966-993:⚠️ Potential issue | 🟠 MajorPotential NPE when
row.getBill()is null.The new date-fallback logic (lines 958–962) correctly handles
row.getBill() == null, but lines 968, 974, 981, and 991 dereferencerow.getBill()without a null guard. If a row has a payment but no direct bill reference, these lines will throw aNullPointerException.🤖 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/PdfController.java` around lines 966 - 993, In PdfController, several cells (the insId cell, conditional Bill Type cell, patientName extraction, and Net Total cell) dereference row.getBill() without a null check and can NPE when row.getBill() == null; fix by first assigning Bill bill = row.getBill() and guard subsequent accesses (use empty string for textual cells and "0.00" or String.format for numeric cells) before calling bill.getInsId(), bill.getBillTypeAtomic(), bill.getPatient(), bill.getNetTotal(), and only render the conditional bill type cell when includeBillType && bill != null; ensure patientName logic checks bill != null and bill.getPatient() != null before calling getPerson()/getNameWithTitle().
1148-1170:⚠️ Potential issue | 🟠 MajorSame NPE risk:
row.getBill()accessed without null check after null-safe date fallback.Lines 1150, 1155, 1160, and 1170 dereference
row.getBill()directly, but the date logic above (lines 1141–1145) treats it as potentially null. Either guard all accesses or skip the row whenrow.getBill()is null.🤖 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/PdfController.java` around lines 1148 - 1170, The code accesses row.getBill() without null checks in multiple places (used in table.addCell blocks for Bill No, Bill Type, Patient, and Net Total) while earlier logic treated row.getBill() as possibly null; either add a guard at the start of the loop (e.g., if (row.getBill() == null) continue;) to skip rows with null bills, or wrap each usage (row.getBill().getDeptId(), row.getBill().getBillTypeAtomic(), row.getBill().getPatient(), row.getBill().getNetTotal()) with null-safe checks before dereferencing and provide sensible defaults (empty strings or 0.00) to avoid NPEs. Ensure patientName assignment also checks row.getBill().getPatient() and its getPerson() for null before calling getNameWithTitle().
🧹 Nitpick comments (2)
src/main/java/com/divudi/bean/common/PdfController.java (2)
774-776:paymentReportStaffWelfareandpaymentReportVouchersilently produce no PDF output.These cases break without rendering anything. If this is intentional (e.g., not yet implemented), consider adding a comment or a "No Data" placeholder so the omission is visible in the generated PDF.
🤖 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/PdfController.java` around lines 774 - 776, The switch in PdfController currently has cases "paymentReportStaffWelfare" and "paymentReportVoucher" that simply break and produce no PDF output; update the handling inside the method containing that switch so these cases either render a clear placeholder PDF page (e.g., "No Data / Report not implemented") or explicitly log/throw an identifiable exception (e.g., UnsupportedOperationException) to avoid silent failures. Locate the switch by the case labels "paymentReportStaffWelfare" and "paymentReportVoucher" and implement consistent behavior with the other report cases (placeholder rendering or clear error/ log), ensuring the change is visible in generated PDFs or server logs.
2342-2411: Unusedtypeparameter inaddDtoDataToPdf.The
typeparameter is accepted but never referenced in the method body. All bundles render the same 7-column layout regardless of type. If different bundle types need different table layouts (as suggested by the variety of bundle types this PR adds), this parameter should drive the rendering. Otherwise, remove it to avoid confusion.🤖 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/PdfController.java` around lines 2342 - 2411, The method addDtoDataToPdf declares an unused parameter type which is confusing; either remove the type parameter from addDtoDataToPdf (and any callers) if all bundles share the same 7-column layout, or use the type value to branch rendering logic (e.g., switch on type inside addDtoDataToPdf to choose different header arrays, column widths and row cell population) so different bundle types produce different tables; update any callers and related helper addDtoBundleTotal if needed to match the new signature or behavior.
🤖 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/PdfController.java`:
- Around line 956-963: Replace direct Date.toString() calls in PdfController
(where dateTimeStr is set from row.getPayment().getBill().getCreatedAt() or
row.getBill().getCreatedAt()) with a formatted string using the same
SimpleDateFormat pattern used in populateTableForChequePayments ("dd MMM yyyy
hh:mm a"); instantiate the SimpleDateFormat once before the loop to avoid
per-row allocation, then call format(createdAt) when createdAt is non-null and
fall back to "N/A" otherwise; apply the same change to the other similar spots
in PdfController that currently call getCreatedAt().toString().
---
Outside diff comments:
In `@src/main/java/com/divudi/bean/common/PdfController.java`:
- Around line 738-739: The orphan empty 6-column Table created by "Table table =
new Table(new float[]{10, 40, 15, 15, 10, 10});" and the unconditional
"document.add(table)" are always appended after each branch; move the creation
and the document.add(table) call into the default branch so only the default
case uses that table. Update the PdfController method where the switch/case
creates per-branch tables (the new cases at lines ~829–849) to remove the shared
table variable and remove the unconditional document.add(table) at the end;
instead instantiate the table and call document.add(table) inside the default
case only.
- Around line 966-993: In PdfController, several cells (the insId cell,
conditional Bill Type cell, patientName extraction, and Net Total cell)
dereference row.getBill() without a null check and can NPE when row.getBill() ==
null; fix by first assigning Bill bill = row.getBill() and guard subsequent
accesses (use empty string for textual cells and "0.00" or String.format for
numeric cells) before calling bill.getInsId(), bill.getBillTypeAtomic(),
bill.getPatient(), bill.getNetTotal(), and only render the conditional bill type
cell when includeBillType && bill != null; ensure patientName logic checks bill
!= null and bill.getPatient() != null before calling
getPerson()/getNameWithTitle().
- Around line 1148-1170: The code accesses row.getBill() without null checks in
multiple places (used in table.addCell blocks for Bill No, Bill Type, Patient,
and Net Total) while earlier logic treated row.getBill() as possibly null;
either add a guard at the start of the loop (e.g., if (row.getBill() == null)
continue;) to skip rows with null bills, or wrap each usage
(row.getBill().getDeptId(), row.getBill().getBillTypeAtomic(),
row.getBill().getPatient(), row.getBill().getNetTotal()) with null-safe checks
before dereferencing and provide sensible defaults (empty strings or 0.00) to
avoid NPEs. Ensure patientName assignment also checks row.getBill().getPatient()
and its getPerson() for null before calling getNameWithTitle().
---
Nitpick comments:
In `@src/main/java/com/divudi/bean/common/PdfController.java`:
- Around line 774-776: The switch in PdfController currently has cases
"paymentReportStaffWelfare" and "paymentReportVoucher" that simply break and
produce no PDF output; update the handling inside the method containing that
switch so these cases either render a clear placeholder PDF page (e.g., "No Data
/ Report not implemented") or explicitly log/throw an identifiable exception
(e.g., UnsupportedOperationException) to avoid silent failures. Locate the
switch by the case labels "paymentReportStaffWelfare" and "paymentReportVoucher"
and implement consistent behavior with the other report cases (placeholder
rendering or clear error/ log), ensuring the change is visible in generated PDFs
or server logs.
- Around line 2342-2411: The method addDtoDataToPdf declares an unused parameter
type which is confusing; either remove the type parameter from addDtoDataToPdf
(and any callers) if all bundles share the same 7-column layout, or use the type
value to branch rendering logic (e.g., switch on type inside addDtoDataToPdf to
choose different header arrays, column widths and row cell population) so
different bundle types produce different tables; update any callers and related
helper addDtoBundleTotal if needed to match the new signature or behavior.
1 participant
Summary by CodeRabbit