Merged
Conversation
Signed-off-by: Pubudu <pubudupiyankara.me@gmail.com>
Signed-off-by: Pubudu <pubudupiyankara.me@gmail.com>
… #19440_profit_matrix_bugs
Closed
9 tasks
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a patient-autocomplete returning discharged encounters with finalized payments, introduces PatientEncounterDto and JSF converters, updates profit-matrix report/querying to use DTO-backed filters and room-category data, adds PDF export endpoints, and updates related UI to bind to DTOs and converters. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Profit Matrix UI
participant Report as ReportController
participant Conv as PatientEncounterDtoConverter
participant Facade as PatientEncounterFacade
participant DB as Database
UI->>Report: User selects/autocompletes patient (binds PatientEncounterDto)
Report->>Conv: getAsObject(string id)
Conv->>Facade: find(id)
Facade->>DB: SELECT PatientEncounter by id
DB-->>Facade: PatientEncounter entity
Facade-->>Conv: PatientEncounter
Conv-->>Report: PatientEncounterDto
Report->>Report: Build JPQL using DTO (e.g., bhtNo equality)
Report->>Facade: findLightsByJpql(query, params, ..., limit)
Facade->>DB: Execute JPQL
DB-->>Facade: Result list
Facade-->>Report: List<PatientEncounterDto>
UI->>Report: downloadDetailPdf()
Report->>Report: Validate rows non-empty, generate PDF stream
Report-->>UI: PDF response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/java/com/divudi/bean/report/ReportController.java (1)
3159-3171: Make these new PDF exports atomic.Both methods reset the response and stream the PDF directly into it. If generation fails after bytes have started flowing, the user gets a partial download and JSF cannot surface a normal error. Buffer into a
ByteArrayOutputStreamfirst, then write the finished PDF in one pass.Based on learnings: "Guideline: For atomic Amazon downloads (Excel/PDF) ensure you serialize the complete document into a ByteArrayOutputStream first, then call response.reset(), set the Content-Length, and write the bytes to the response output stream in a single pass. This prevents partial downloads if generation fails mid-stream and allows your framework (e.g., JSF) to display an error. Apply this pattern to similar controllers/endpoints that produce binary downloads."
Also applies to: 3290-3302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/divudi/bean/report/ReportController.java` around lines 3159 - 3171, The PDF streaming must be made atomic: instead of creating the PdfWriter on the servlet response OutputStream (see Document document, PdfWriter.getInstance(document, out), document.open(), OutputStream out, FacesContext.getCurrentInstance()/ExternalContext), generate the PDF into a ByteArrayOutputStream first (create PdfWriter on that BAOS and write/close document), then call externalContext.responseReset(), set response headers including Content-Length (bytes.length), and finally write the BAOS.toByteArray() to externalContext.getResponseOutputStream() in one pass; apply the same change to the other export method referenced around the Pdf generation at lines ~3290-3302.
🤖 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/inward/AdmissionController.java`:
- Around line 792-805: The autocomplete should ignore whitespace-only input: in
AdmissionController where query is used to build the JPQL (variables sql, h and
the call patientEncounterFacade.findLightsByJpql), trim the query string first
(e.g., query = query == null ? null : query.trim()), and if the trimmed query is
null or empty set suggestions = new ArrayList<>() and skip building/executing
the JPQL; only populate h.put("q", "%" + query.toLowerCase() + "%") and call
findLightsByJpql when the trimmed query is non-empty.
In `@src/main/java/com/divudi/bean/report/ReportController.java`:
- Around line 3365-3408: The footer incorrectly sums per-row percentages
(totalMatrix += row.getMatrixPercentage()), producing wrong results; stop
aggregating raw percentages and instead compute a single aggregate percentage
(e.g., weighted by amounts) or leave it blank. Update the loop to NOT accumulate
row.getMatrixPercentage() into totalMatrix; after the loop compute the aggregate
matrix percent (for example: if you want a weighted percent, calculate
aggregateMatrixPercent = totalServiceVal != 0 ? (totalProfit / totalServiceVal)
* 100 : null) and pass that value to addPdfNumberCellBordered (or render an
empty cell) so the grand total shows a correct weighted percentage rather than a
sum of row percentages; adjust references to totalMatrix,
row.getMatrixPercentage(), addPdfNumberCellBordered and the grand total cell
accordingly.
- Around line 3027-3030: The summary/detail queries are using inconsistent
patient filters (exact per.name = :pn vs a different LIKE in the detail query),
causing different row sets; update both query-building sites that reference
patientEncounterDto (the block shown and the similar block at 3122-3135) to use
one stable unique identifier field from PatientEncounterDto (e.g.,
patientEncounterDto.getPhn() or patientEncounterDto.getPatientId()) and add a
single param key (like "patientKey") to params; replace occurrences of
per.name/LIKE on display text with the unique-field predicate and bind the same
param in both summary and detail query builders so both modes filter
identically.
- Around line 3072-3074: The LEFT JOIN to the one-to-many collection
itemFeesAuto is causing duplicate BillItem rows and inflating totals
(bi.netValue summed multiple times via row.getFinalAmount()/totalNetTotal); fix
by removing the problematic join (delete the ".append(\"LEFT JOIN i.itemFeesAuto
itemFee \")" fragment in ReportController) when fees aren't needed in the detail
query, or if a fee column is required join only a single fee per item (e.g.,
restrict to the MIN(id) or first fee via a subquery/derived selection) so each
BillItem appears once; update any logic that referenced itemFee accordingly and
re-run totals to confirm totalNetTotal is correct.
In `@src/main/webapp/reports/financialReports/profit_matrix_report.xhtml`:
- Around line 124-141: The autocomplete components backed by PatientEncounterDto
(the p:autoComplete with id="acBht" bound to
#{reportController.patientEncounterDtoForBhtNo} and the other DTO-backed
autocomplete around lines 149-165) need forceSelection="true" added so the
converter (#{patientEncounterDtoConverter}) never receives free-text input that
it cannot parse; update both p:autoComplete tags to include
forceSelection="true" to ensure only selected suggestion objects are submitted.
---
Nitpick comments:
In `@src/main/java/com/divudi/bean/report/ReportController.java`:
- Around line 3159-3171: The PDF streaming must be made atomic: instead of
creating the PdfWriter on the servlet response OutputStream (see Document
document, PdfWriter.getInstance(document, out), document.open(), OutputStream
out, FacesContext.getCurrentInstance()/ExternalContext), generate the PDF into a
ByteArrayOutputStream first (create PdfWriter on that BAOS and write/close
document), then call externalContext.responseReset(), set response headers
including Content-Length (bytes.length), and finally write the
BAOS.toByteArray() to externalContext.getResponseOutputStream() in one pass;
apply the same change to the other export method referenced around the Pdf
generation at lines ~3290-3302.
🪄 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: cf6374e9-6f32-406b-8317-1cb3a6928d7c
📒 Files selected for processing (8)
src/main/java/com/divudi/bean/inward/AdmissionController.javasrc/main/java/com/divudi/bean/inward/AdmissionTypeController.javasrc/main/java/com/divudi/bean/report/ReportController.javasrc/main/java/com/divudi/core/converter/PatientEncounterDtoConverter.javasrc/main/java/com/divudi/core/converter/RoomCategoryConverter.javasrc/main/java/com/divudi/core/data/dto/PatientEncounterDto.javasrc/main/java/com/divudi/core/data/dto/ProfitMatrixRowDTO.javasrc/main/webapp/reports/financialReports/profit_matrix_report.xhtml
buddhika75
approved these changes
Apr 4, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Style