O3-5370: Auto-Generate Billable Items from Medication Orders#156
O3-5370: Auto-Generate Billable Items from Medication Orders#156wikumChamith wants to merge 2 commits intoopenmrs:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
============================================
+ Coverage 26.73% 28.71% +1.98%
- Complexity 487 520 +33
============================================
Files 190 192 +2
Lines 4309 4328 +19
Branches 486 471 -15
============================================
+ Hits 1152 1243 +91
+ Misses 3056 2972 -84
- Partials 101 113 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| try { | ||
| switch (order.getAction()) { | ||
| case NEW: | ||
| return handleNewOrder(order); |
There was a problem hiding this comment.
Neither handleNewOrder() in the DrugOrderBillingStrategy and TestOrderBillingStrategy implementation checks if a BillLineItem already exists for the order. Its mentioned in the interface Javadoc that implementations should do that -
| } | ||
| } | ||
| catch (Exception e) { | ||
| log.error("Error processing order (action={}): {}", order.getAction(), e.getMessage(), e); |
There was a problem hiding this comment.
Sort of nit, but doesn’t this make it hard to catch errors because anything even a deliberate "no billable service configured" all would produce the exact same output? Should we let unexpected exceptions propagate or be caught and rethrown as a specific runtime exception with actionable context (order UUID, action, strategy name)?
|
|
||
| for (OrderBillingStrategy strategy : strategies) { | ||
| if (strategy.supports(order)) { | ||
| strategy.generateBill(order); |
There was a problem hiding this comment.
strategy.generateBill(order) is called but the Optional<Bill> result is never inspected. There is no log line for a successful bill creation and no way to distinguish a deliberate skip from a silent failure. Should we log the outcome — bill.getUuid() on success, or a named reason on empty?
| existingLineItem.setVoided(true); | ||
| existingLineItem.setVoidReason(reason); | ||
| existingLineItem.setDateVoided(new Date()); | ||
| existingLineItem.setVoidedBy(Context.getAuthenticatedUser()); |
There was a problem hiding this comment.
since setVoidedBy(Context.getAuthenticatedUser()) runs inside Daemon.runInDaemonThread(), wouldn’t the authenticated user get set as the daemon?
There was a problem hiding this comment.
Can we add some tests for this?
| protected abstract Optional<Bill> handleNewOrder(Order order); | ||
|
|
||
| private Optional<Bill> handleRevisedOrder(Order order) { | ||
| voidPreviousLineItem(order, "Order revised"); |
There was a problem hiding this comment.
voidPreviousLineItem and handleNewOrder are separate service calls with no transactional wrapper (daemon thread,no @Transactional). If handleNewOrder fails after voidPreviousLineItem succeeds, the old bill is voided with no replacement.
| * Strategy for generating a bill from an order. Implementations are Spring beans registered by | ||
| * name. To override the default behavior for a given order type, register a bean with the same name | ||
| * (e.g. "drugOrderBillingStrategy") and mark it {@code @Primary}, or register a custom bean and | ||
| * give it a higher {@code @org.springframework.core.annotation.Order} priority. | ||
| */ |
There was a problem hiding this comment.
Does processOrder() sort by @order. Does it just iterate whatever Spring returns and does @Primary affect getRegisteredComponents()?
| DrugOrder drugOrder = (DrugOrder) order; | ||
|
|
||
| StockManagementService stockService = Context.getService(StockManagementService.class); | ||
| Integer drugId = drugOrder.getDrug() != null ? drugOrder.getDrug().getDrugId() : 0; |
There was a problem hiding this comment.
This might have been what had been there till now, but do we really want to set the drug id to zero if the drug id is null?
3 participants
Ticket: https://openmrs.atlassian.net/browse/O3-5372?focusedCommentId=191765