Index Bill and BillRefund into querystore via the SPI - #175
Open
dkayiwa wants to merge 9 commits into
Open
Conversation
Implements org.openmrs.module.querystore.spi.ResourceTypeProvider for two billing resource types — billing_bill and billing_bill_refund — producing the cross-cutting QueryDocument fields (patient_uuid, resource_uuid, date, last_modified, text) plus type-specific metadata (status, totals, balance, cashier, cash point, visit; refund amount, approver/completer, dates). Re-indexing is wired through AbstractIndexingAdvice subclasses on BillService and BillRefundService — each save/void/unvoid/purge call projects the entity through the embed-then-upsert pipeline after commit. BillDiscountServiceImpl.saveBillDiscount additionally re-saves the parent Bill so amount_after_discount (denormalized into the bill document) stays in sync after discount mutations; the BillService advice picks up the resulting save. BillLineItem and Payment are intentionally not contributed as standalone types — neither has a service save method (line items cascade-save from Bill, payments have no service at all), so AOP can't trigger on their mutations. Their state is summarized inside the Bill document instead. Balance is derived from getAmountAfterDiscount, not gross total, so a bill whose effective amount is fully covered by payments reports a zero/negative balance correctly. Querystore is declared require_module rather than aware_of, because the advice and provider classes statically reference querystore-api types; without querystore on the classpath the bean classes fail to load and Spring context init fails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dkayiwa
force-pushed
the
querystore-contributor
branch
from
May 19, 2026 22:46
66b0d5a to
d35e723
Compare
dkayiwa
changed the title
billing-api now declares serializer / provider beans whose supertypes live in querystore-api (provided scope). Spring's AOP infrastructure resolves every bean's class during context init to check for Advisor candidates, so lazy-init does not skip the type check — the FHIR module's tests load billing-api's moduleApplicationContext.xml and crashed with NoClassDefFoundError on AbstractRecordSerializer because provided-scope deps don't propagate to dependent modules' test classpaths. Adds querystore-api as a test-scope dependency in fhir/pom.xml so the test classpath is self-sufficient. The omod packaging is unaffected — querystore-api is still provided at the API layer, so it isn't bundled. Also adds lazy-init on the four querystore beans in moduleApplicationContext.xml. This doesn't fix the Advisor-scan path above, but it does prevent unrelated bean lookups from forcing the classes to load at startup when querystore is genuinely absent at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 1 — test coverage:
- Adds 7 unit tests for BillRecordSerializer including the
balance-from-amountAfterDiscount correctness invariant, and 8 for
BillRefundRecordSerializer (core fields, metadata, optional refs,
null guards, line-scoped vs bill-scoped refunds).
Phase 1 — re-entry, real correctness fix:
- BillLineItemServiceImpl.voidBillLineItem now re-saves the parent
Bill so dateChanged advances and the BillIndexingAdvice picks up
the resulting save. Without this, voiding a line item changed the
bill's getTotal() / getAmountAfterDiscount() / getTotalPayments()
without touching any bill column, leaving the indexed Bill document
stale. Symmetric to the existing discount-touch fix.
Phase 2 — polish:
- BillRefundIndexingAdvice TRIGGER_METHODS shrunk to {saveBillRefund}
and PURGE_METHODS to empty. The previous set named void/unvoid/purge
methods that don't exist on BillRefundService — aspirational coverage
that AOP could never match.
- BillRefundRecordSerializer skips refunds with null refundAmount
instead of NPE-ing inside the AOP advice's per-entity exception
swallow (which would silently drop the refund from the index).
- touchParentBill log.error -> log.warn for the recoverable
"bill could not be loaded" race in both BillDiscountServiceImpl
and the new BillLineItemServiceImpl.
Deferred to follow-up: extracting the privilege+load+save pattern now
duplicated in 4 places (touches code outside this slice); investigating
Bill.hbm.xml cascade-delete on refunds (pre-existing entity-mapping
concern); the constructor-injection-vs-bean-name lookup divergence
between providers and advice (both shapes work, the SPI's documented
pattern uses bean-name lookup).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ices A typo in BillIndexingAdvice.TRIGGER_METHODS or BillRefundIndexingAdvice's matching set produces no compile error, no startup error, and no runtime exception — AbstractIndexingAdvice matches by method name, and a name that doesn't resolve simply never fires. The failure surfaces only as "stale rows in the read store" some time later. This was exactly the bug shape we corrected on BillRefundIndexingAdvice this cycle when it listed voidBillRefund / unvoidBillRefund / purgeBillRefund — methods that don't exist on BillRefundService. The reflection test now asserts every name in the trigger and purge sets resolves to a method on the target service interface and that purge methods are a subset of trigger methods (per the AbstractIndexingAdvice contract). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Bill and BillRefund documents previously carried only IDs and totals — they had no signal a search like "which bills include item X" could match against. The serializers now collect each non-voided line item's display name (BillableService.name first, falling back to StockItem.commonName) and emit them into both the searchable text blob and a new `line_item_names` metadata field shaped as `List<String>` to match the querystore module's convention for multi-valued fields (VisitRecordSerializer's encounter_uuids, AllergyRecordSerializer's reactions). The refund serializer emits a singleton list under the same key — refunds are line-scoped — and preserves the line item's name even when that line item has since been voided on the parent bill, because a refund is an audit record of past activity. The display-name lookup is extracted into a package-private helper scoped to the querystore SPI. It is intentionally NOT the source for ReceiptGenerator (which prefers Drug.name on printed receipts) or the FHIR translator (which keys off Concept presence); "consolidating" the three consumers would silently change user-visible receipts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Bill document now carries the data needed to answer ops/finance queries that the prior index could not: payment_modes (distinct sorted tender names for "settlements by Mobile Money"), discount_statuses (presence query for "bills with a pending discount"), bill_adjusted_uuid and adjusted_by_uuids (chain navigation in both directions), adjustment_reason, and receipt_printed (always-emitted boolean for "paid bills not yet printed"). Aggregations sort their output so the same logical state produces bytewise-identical documents across reindexes — Bill.payments / Bill.discounts / Bill.adjustedBy are HashSets, so without the sort the document bytes drift on every save. A new resource type billing_bill_discount is also indexed end-to-end (serializer + AOP advice on saveBillDiscount + ResourceTypeProvider + Spring wiring). It exists so the "approval queue" question — show me all pending discounts requiring my review — can be answered with one patient-scoped query against the discount documents, rather than scanning every bill's denormalized discount_statuses aggregate. The discount document also carries the canonical discount_amount (computed money figure) alongside discount_value (raw input) so callers can distinguish "find 15% discounts" from "find discounts > $50". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Bill document now carries the workflow and clinical-link data the
prior indexes were missing: line_item_statuses lets "find bills with a
REFUND_REQUESTED line item" succeed without scanning every line item
row; order_uuids closes the clinical-to-billing trace ("find the bill
for this lab order"); cashier_name denormalizes Provider.getName() so
admin queries don't pay a second lookup; payment_mode_amounts ships
parallel to payment_modes (same TreeMap alphabetic order) so consumers
can zip the two arrays at query time to answer "total Cash collected
this week" — payments of the same mode are summed into one entry.
A shared BillingAuditFields helper centralises the OpenMRS audit
columns (created_at, date_changed, date_voided, creator_uuid,
changed_by_uuid, voided_by_uuid, void_reason) across the four
BaseOpenmrsData resource types — emitting them inline would invite
drift where a future refactor adds the field to one serializer and
forgets the others, and audit queries silently miss the new type.
The fifth indexed resource type billing_timesheet ships end-to-end:
TimesheetRecordSerializer (provider-scoped — patientUuid is null by
design), TimesheetIndexingAdvice (triggers on the generic
IEntityDataService surface: save / voidEntity / unvoidEntity / purge),
TimesheetResourceTypeProvider, Spring wiring, advice point. It lets
"who was on duty between 2pm-3pm" succeed by querying clock_in /
clock_out without scanning every timesheet row.
BillableService catalog indexing was attempted but dropped:
AbstractIndexingAdvice<T extends BaseOpenmrsData> rules it out at the
type-bound level. The querystore SPI is patient-data-scoped by design;
widening the bound to admit BaseChangeableOpenmrsMetadata is a
querystore change, not a billing change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The querystore-api jar (now a require_module per omod/config.xml) is compiled with Java 11 bytecode (class version 55). A Java 8 build couldn't load it at runtime regardless of source level, so keeping maven.compiler.source/target=8 only created the illusion of broader compatibility. The CI matrix drops the Java 8 cell for the same reason — it would only ever surface a compatibility error that doesn't apply to any deployment that can actually use the querystore slice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The broader timesheet feature is being removed in PR #164 (cashier HR concern, not a billing concern), so indexing timesheets into the querystore would ship a resource type whose source data is about to disappear. Co-Authored-By: Claude Opus 4.7 (1M context) <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
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
org.openmrs.module.querystore.spi.ResourceTypeProviderfor two billing resource types —billing_billandbilling_bill_refund— extendingAbstractRecordSerializer<T>to produce the cross-cuttingQueryDocumentfields plus type-specific metadata.AbstractIndexingAdvicesubclasses onBillServiceandBillRefundServiceso save / void / unvoid / purge calls re-index through the querystore embed-then-upsert pipeline after commit.BillDiscountServiceImpl.saveBillDiscountnow re-saves the parent Bill —amount_after_discountis denormalized into the bill document but derived fromBillDiscountrows, so a discount mutation needs to trigger the parent'sBillService.saveBillfor the AOP advice to fire.getAmountAfterDiscount, not gross total, so a discounted bill that's fully paid reports a zero/negative balance correctly.Notes for reviewers
querystoreis nowrequire_module, notaware_of. The advice and provider classes statically referencequerystore-apitypes; without querystore on the classpath the Spring context init fails. If you need billing to work without querystore, that's a separate (non-trivial) slice involving conditional bean loading.BillLineItemandPaymentare intentionally not contributed as standalone resource types. Line items have no save method onBillLineItemService(cascade-save from Bill only);Paymenthas no service at all. AOP only fires on service calls, so an indexedbilling_bill_line_itemorbilling_paymentwould be a partial feature where new rows don't appear until bootstrap. They're summarized inside the Bill document'stextand metadata instead.querystore-apidependency is pinned to1.0.0-SNAPSHOT— the published snapshot atmavenrepo.openmrs.org/public. Parent pom now enables snapshots on the OpenMRS public repo so the build resolves it.Test plan
mvn -pl api compile— passesmvn -pl api test -Dtest=BillDiscountServiceImplTest#saveBillDiscount_shouldAdvanceParentBillDateChanged— passes; new test asserts the parent bill'sdateChangedadvances on a discount saveBillDiscountServiceImplTest— passes (no regressions)Billand confirm thebilling_billdocument is produced with expected fields (patient_uuid,receipt_number,status,total,amount_after_discount,total_paid,balance,cashier_uuid,cash_point_uuid,visit_uuid)BillDiscount(approve / void) → confirm the parentBilldocument is re-indexed with freshamount_after_discountandbalanceBillRefund→ confirm thebilling_bill_refunddocument is produced and re-indexed on subsequent updates🤖 Generated with Claude Code