fix(pharmacy): prevent NPEs and over-issuing in transfer settlement#21451
Merged
Conversation
…tity
When a bill item's available stock was less than the requested quantity,
the insufficient-stock error message navigated through a minimal ItemBatch
object (built from DTO data without the Item relationship populated) to
retrieve the item name, causing a NullPointerException:
bi.getPharmaceuticalBillItem().getItemBatch().getItem().getName()
^^^^^^^^^^^ null
Root cause: createBillItemFromStockDTO() constructs a minimal ItemBatch
from bulk-query DTO data (id, batchNo, rates, expiry) but never calls
batch.setItem(...), leaving the item field null.
Fix: use bi.getItem().getName() instead — BillItem.item is always set
correctly via newBillItem.setItem(referenceItem.getItem()) during bill
item creation and is updated by replaceSelectedSubstitute() when a
substitute is chosen.
Diagnosed from server log: NullPointerException at
TransferIssueForRequestsController.settle(TransferIssueForRequestsController.java:422)
Closes #20972
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ent NPE The previous fix replaced getItemBatch().getItem().getName() with bi.getItem().getName(), but bi.getItem() or getName() can also be null (e.g. legacy data, Vmpp/Vmp items). Remove the item name reference entirely and use a safe generic message instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two independent bugs allowed a transfer issue to proceed even when actual DB stock was insufficient: 1. Stale in-memory stock check The validation loop compared bi.getPharmaceuticalBillItem().getStock() .getStock() against qty, but that Stock object is constructed at page-load time by createBillItemFromStockDTO() with the available qty captured from the bulk DTO query. Any stock movement that occurs after the page loads (retail sale, concurrent issue) is invisible to this check. Fix: replace with a live DB lookup via StockFacade.find() for every item immediately before saveBill() is called, so the check always reflects the true current stock level. 2. Negated qty passed to isStockAvailable() In the post-save processing loop, qty is negated at line 831 before isStockAvailable() is called at line 856. The internal check (qty > fetchedStock.getStock()) evaluates as e.g. -10 > 2 → false, causing the guard to always return true regardless of actual stock. Fix: pass Math.abs(tmpPh.getQty()) so the comparison uses the magnitude of the quantity. Also made the error messages null-safe (bi.getItem() null guard) and added a clearer message telling the user to refresh when live stock check fails, since the cause is stale page state. Closes #20972 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…data
When a transfer issue is settled, the PharmaceuticalBillItem carries a
minimal ItemBatch built from a DTO — only id, batchNo, and rates are set;
the item relationship is null. Both addToStock(pbi, qty, Staff) and
deductFromStock(pbi, qty, Staff) create a new Staff Stock row when none
exists, and they attempt to populate that row's metadata from the batch:
ItemBatch ib = pharmaceuticalBillItem.getItemBatch();
Item i = ib != null ? ib.getItem() : null;
if (i != null) { /* set itemName, barcode, longCode, dateOfExpire, retailsaleRate */ }
else { s.setItemName("UNKNOWN"); s.setBarcode(""); s.setLongCode(0L); }
Because the DTO batch has no item loaded, i is always null, so every Staff
Stock row created through a transfer issue is stored with:
- ITEM_NAME = "UNKNOWN"
- BARCODE = ""
- LONGCODE = 0
- DATEOFEXPIRE = null
- RETAILSALE_RATE = 0.0 (skipped in the else branch)
This does not break stock quantity movement — the JPQL lookup key is
(itemBatch, staff) by ID — but it corrupts the metadata fields used by
stock valuation and expiry reports for in-transit staff stock.
Fix: before accessing ib.getItem(), check whether the item relation is
unloaded (null) while the batch has a valid DB id. If so, call
itemBatchFacade.find(ib.getId()) to obtain the full entity. The loaded
entity is used only for metadata population; the FK already stored on
the Stock row is unchanged. The guard (getItem() == null && getId() != null)
ensures no extra query is issued when the full entity is already loaded
(normal receive-side path where ItemBatch is eagerly fetched from DB).
Applied to both:
- addToStock(PharmaceuticalBillItem, double qty, Staff)
- deductFromStock(PharmaceuticalBillItem, double qty, Staff)
Closes #20972
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 11 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR adds live database stock verification in TransferIssueForRequestsController.settle() and makes PharmacyBean reload minimal ItemBatch entities before using batch/item fields during add/deduct stock operations. ChangesStock Validation Reliability
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ 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 |
…aved-with-empty-data # Conflicts: # src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java
![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: 3277fb71b5
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java (1)
804-804: 💤 Low valueConsider using
Math.abs()for defensive stock comparison.The comparison
liveStock.getStock() < pbi.getQty()assumespbi.getQty()is positive. While in the current flow this should be true (qty is negated later at line 847), usingMath.abs()would be defensive and consistent with the fix at line 872.- if (liveStock == null || liveStock.getStock() < pbi.getQty()) { + if (liveStock == null || liveStock.getStock() < Math.abs(pbi.getQty())) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java` at line 804, Change the stock check to defensively compare absolute quantity values: replace the conditional if (liveStock == null || liveStock.getStock() < pbi.getQty()) with one that uses Math.abs(pbi.getQty()), e.g. if (liveStock == null || liveStock.getStock() < Math.abs(pbi.getQty())), so liveStock.getStock() is compared to the absolute requested quantity (referencing liveStock, getStock(), and pbi.getQty()) to match the defensive pattern used later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java`:
- Around line 790-821: The loop over getBillItems() can NPE because
createEmptyBillItem() may leave pharmaceuticalBillItem or billItemFinanceDetails
null; add null checks: when you fetch PharmaceuticalBillItem pbi =
bi.getPharmaceuticalBillItem() guard before using pbi (e.g., if pbi == null ->
add a user error or skip this BillItem) and likewise guard
bi.getBillItemFinanceDetails() before calling getQuantity() (treat null as zero
or surface an error). Update the block that uses pbi.getItemBatch(),
pbi.getStock(), and bi.getBillItemFinanceDetails().getQuantity() (and any places
calling getRemainingQuantityForItem(bi.getReferanceBillItem())) to handle nulls
deterministically so iteration does not throw NPEs.
---
Nitpick comments:
In
`@src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java`:
- Line 804: Change the stock check to defensively compare absolute quantity
values: replace the conditional if (liveStock == null || liveStock.getStock() <
pbi.getQty()) with one that uses Math.abs(pbi.getQty()), e.g. if (liveStock ==
null || liveStock.getStock() < Math.abs(pbi.getQty())), so liveStock.getStock()
is compared to the absolute requested quantity (referencing liveStock,
getStock(), and pbi.getQty()) to match the defensive pattern used later.
🪄 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: ecd21b25-5eba-4bb0-b2a9-e3d68207baad
📒 Files selected for processing (2)
src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.javasrc/main/java/com/divudi/ejb/PharmacyBean.java
…e lines deductFromStock() already does an atomic UPDATE...WHERE s.stock >= :qty check (the #20138 TOCTOU-safe guard) and returns false if insufficient, handled by the existing else branch. The userStockController.isStockAvailable() check ran after the BillItem/PharmaceuticalBillItem were already persisted, and on failure only zeroed tmpQty and continued - leaving a saved transfer issue line with no corresponding stock movement. UserStockContainer reservations are not populated in this flow, so the check added no real protection beyond the live stock check already done earlier in settle(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ceuticalBillItem createEmptyBillItem() (used in generateBillComponent when an item has no available stock) adds a BillItem with pharmaceuticalBillItem left unset. Both the live stock validation loop and the zero-qty filter in settle() dereferenced getPharmaceuticalBillItem() unconditionally, NPEing whenever such an item was present in the transfer issue. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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
settle()when actual stock is below the requested quantityItemBatchbefore populating staff Stock metadata so transfer-issue stock rows no longer getUNKNOWN/empty metadataCloses #20972
🤖 Generated with Claude Code
Summary by CodeRabbit