Skip to content

O3-5197: Each payment should have an associated Cashier #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wikumChamith wants to merge 2 commits into
base: main
Choose a base branch
from
Open

O3-5197: Each payment should have an associated Cashier #109

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e458d7
O3-5197: Each payment should have an associated Cashier
wikumChamith Feb 10, 2026
608b250
Update the messages.properties
wikumChamith Feb 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/src/main/java/org/openmrs/module/billing/api/model/Payment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.Setter;
import org.openmrs.Provider;
import org.openmrs.module.billing.api.base.entity.model.BaseInstanceCustomizableData;

import java.math.BigDecimal;
Expand Down Expand Up @@ -46,6 +47,10 @@ public class Payment extends BaseInstanceCustomizableData<PaymentMode, PaymentAt
@Setter
private BigDecimal amountTendered;

@Getter
@Setter
private Provider cashier;
Copy link
Member

@ibacher ibacher Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to make this a User rather than Provider?

Copy link
Member Author

@wikumChamith wikumChamith Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but Bill is using Provider.

openmrs-module-billing/api/src/main/java/org/openmrs/module/billing/api/model/Bill.java

Line 46 in 3d15b7a

private Provider cashier;

Copy link
Contributor

@NethmiRodrigo NethmiRodrigo Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think either should be a provider...

Copy link
Member Author

@wikumChamith wikumChamith Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should do a last-minute data model change 🙂 Let’s do this after the release.

Copy link
Contributor

@NethmiRodrigo NethmiRodrigo Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't something we need in the release, I think we should merge this whole thing in after the release


public Integer getId() {
return paymentId;
}
Expand Down
21 changes: 21 additions & 0 deletions api/src/main/java/org/openmrs/module/billing/validator/BillValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.openmrs.module.billing.api.BillLineItemService;
import org.openmrs.module.billing.api.model.Bill;
import org.openmrs.module.billing.api.model.BillLineItem;
import org.openmrs.module.billing.api.model.Payment;
import org.springframework.validation.Errors;
import org.springframework.validation.Validator;

Expand All @@ -36,6 +37,7 @@ public void validate(@Nonnull Object target, @Nonnull Errors errors) {
}

validateLineItemsNotModified(bill, errors);
validatePaymentsHaveCashier(bill, errors);
}
}

Expand Down Expand Up @@ -82,4 +84,23 @@ private void validateLineItemsNotModified(Bill bill, Errors errors) {
}
}

/**
* Validates that all non-voided payments on the bill have an associated cashier (Provider). This
* ensures accountability by tracking which cashier processed each payment.
*
* @param bill the bill whose payments to validate
* @param errors the errors object to add validation errors to
*/
private void validatePaymentsHaveCashier(Bill bill, Errors errors) {
if (bill.getPayments() == null) {
return;
}
for (Payment payment : bill.getPayments()) {
if (payment != null && !payment.getVoided() && payment.getCashier() == null) {
errors.reject("billing.payment.error.cashierRequired", "Each payment must have an associated cashier");
Copy link
Member

@ibacher ibacher Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding strings referencing codes, it's good to add the code (and string) to the message.properties so we can actually localize it.

Copy link
Member Author

@wikumChamith wikumChamith Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a message.properties file in this module. Should I add a one in this PR, or is it going to be a separate PR?

Copy link
Member

@ibacher ibacher Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just start with this PR.

Copy link
Member Author

@wikumChamith wikumChamith Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibacher Sorry, there is a messages.properties file in this module. For some reason, it didn’t show up in the file search I did earlier. I’ve updated it with the missing values.

return;
}
}
}

}
2 changes: 2 additions & 0 deletions api/src/main/resources/Bill.hbm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@
<property name="amount" type="java.math.BigDecimal" column="amount" not-null="true"/>
<property name="amountTendered" type="java.math.BigDecimal" column="amount_tendered" not-null="true"/>

<many-to-one name="cashier" class="org.openmrs.Provider" column="provider_id" not-null="false"/>

<set name="attributes" lazy="false" inverse="true" cascade="all-delete-orphan">
<key column="bill_payment_id"/>
<one-to-many class="org.openmrs.module.billing.api.model.PaymentAttribute"/>
Expand Down
19 changes: 18 additions & 1 deletion api/src/test/java/org/openmrs/module/billing/impl/BillServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ public void saveBill_shouldAllowPaymentsForPostedBill() {
PaymentMode paymentMode = paymentModeService.getPaymentMode(0);

Payment payment = Payment.builder().amount(BigDecimal.valueOf(10.0)).amountTendered(BigDecimal.valueOf(10.0))
.build();
.cashier(providerService.getProvider(0)).build();
payment.setInstanceType(paymentMode);

postedBill.addPayment(payment);
Expand All @@ -341,6 +341,23 @@ public void saveBill_shouldAllowPaymentsForPostedBill() {
assertDoesNotThrow(Context::flushSession);
}

@Test
public void saveBill_shouldThrowValidationExceptionWhenPaymentHasNoCashier() {
Bill postedBill = billService.getBill(0);
assertNotNull(postedBill);
assertEquals(BillStatus.POSTED, postedBill.getStatus());

PaymentMode paymentMode = paymentModeService.getPaymentMode(0);

Payment payment = Payment.builder().amount(BigDecimal.valueOf(10.0)).amountTendered(BigDecimal.valueOf(10.0))
.build();
payment.setInstanceType(paymentMode);

postedBill.addPayment(payment);

assertThrows(ValidationException.class, () -> billService.saveBill(postedBill));
}

@Test
public void saveBill_shouldNotAllowModifyingLineItemPropertiesOnPaidBill() {
Bill paidBill = billService.getBill(1);
Expand Down
2 changes: 2 additions & 0 deletions api/src/test/resources/org/openmrs/module/billing/api/include/BillTest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
creator="1" date_created="2012-01-01 00:00:00.0" voided="false"
uuid="4028814B39B565A20139B9645D7B0007"/>
<cashier_bill_payment bill_payment_id="0" bill_id="0" payment_mode_id="0" amount="913.06" amount_tendered="1000.00"
provider_id="0"
creator="1" date_created="2012-01-01 00:00:00.0" voided="false"
uuid="4028814B39B565A20139B9674C510008"/>
<cashier_bill_payment_attribute bill_payment_attribute_id="0" bill_payment_id="0" payment_mode_attribute_type_id="0"
Expand All @@ -59,6 +60,7 @@
creator="1" date_created="2012-02-01 00:00:00.0" voided="false"
uuid="5028814B39B565A20139B95FB3440005"/>
<cashier_bill_payment bill_payment_id="1" bill_id="1" payment_mode_id="0" amount="300.00" amount_tendered="300.00"
provider_id="1"
creator="1" date_created="2012-02-01 00:00:00.0" voided="false"
uuid="5028814B39B565A20139B9674C510008"/>

Expand Down
4 changes: 2 additions & 2 deletions .../main/java/org/openmrs/module/billing/web/legacyweb/controller/BillAddEditController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ private Patient getPatientFromService(String patientUuid) {
patient = service.getPatientByUuid(patientUuid);
}
catch (APIException e) {
log.error("Error when trying to get Patient with ID <{}>", patientUuid, e);
log.error("Error when trying to get Patient with ID <{}>", patientUuid, e);
throw new APIException("Error when trying to get Patient with ID <" + patientUuid + ">");
}

Expand All @@ -171,7 +171,7 @@ private Bill getBillFromService(String billUuid) {
bill = service.getBillByUuid(billUuid);
}
catch (APIException e) {
log.error("Error when trying to get bill with ID <{}>", billUuid, e);
log.error("Error when trying to get bill with ID <{}>", billUuid, e);
throw new APIException("Error when trying to get bill with ID <" + billUuid + ">");
}

Expand Down
12 changes: 12 additions & 0 deletions omod/src/main/java/org/openmrs/module/billing/web/rest/resource/PaymentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package org.openmrs.module.billing.web.rest.resource;

import org.openmrs.Provider;
import org.openmrs.api.context.Context;
import org.openmrs.module.billing.api.BillService;
import org.openmrs.module.billing.api.PaymentModeService;
Expand Down Expand Up @@ -55,6 +56,7 @@ public DelegatingResourceDescription getRepresentationDescription(Representation
description.addProperty("attributes");
description.addProperty("amount");
description.addProperty("amountTendered");
description.addProperty("cashier", Representation.REF);
description.addProperty("dateCreated");
description.addProperty("voided");
return description;
Expand All @@ -70,10 +72,20 @@ public DelegatingResourceDescription getCreatableProperties() {
description.addProperty("attributes");
description.addProperty("amount");
description.addProperty("amountTendered");
description.addProperty("cashier");

return description;
}

@PropertySetter("cashier")
public void setCashier(Payment instance, String uuid) {
Provider provider = Context.getProviderService().getProviderByUuid(uuid);
if (provider == null) {
throw new ObjectNotFoundException();
}
instance.setCashier(provider);
}

// Work around TypeVariable issue on base generic property (BaseCustomizableInstanceData.getInstanceType)
@PropertySetter("instanceType")
public void setPaymentMode(Payment instance, String uuid) {
Expand Down
13 changes: 13 additions & 0 deletions omod/src/main/resources/liquibase.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,4 +1095,17 @@
baseTableName="bill_exemption_rule" baseColumnNames="voided_by"
referencedTableName="users" referencedColumnNames="user_id"/>
</changeSet>

<changeSet id="openmrs.billing-004-20260210-add-cashier-to-payment" author="Wikum Weerakutti">
<comment>Add cashier (provider) to each payment for tracking which cashier processed the payment</comment>
<addColumn tableName="cashier_bill_payment">
<column name="provider_id" type="int">
<!--Keeping this nullable for the sake of existing legacy data-->
<constraints nullable="true"/>
</column>
</addColumn>
<addForeignKeyConstraint constraintName="cashier_bill_payment_provider_fk"
baseTableName="cashier_bill_payment" baseColumnNames="provider_id"
referencedTableName="provider" referencedColumnNames="provider_id"/>
</changeSet>
</databaseChangeLog>
Loading