Skip to content

(feat): Payments should be associated with a cashier #155

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
NethmiRodrigo wants to merge 10 commits into
base: main
Choose a base branch
from
+333 −6
Open

(feat): Payments should be associated with a cashier #155

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6436fef
feat: add cashier (Provider) field to Payment model and HBM mapping
NethmiRodrigo Mar 20, 2026
b4fee48
feat: add Liquibase migration for provider_id and error message key
NethmiRodrigo Mar 21, 2026
cf53d79
feat: validate that new payments have an associated cashier
NethmiRodrigo Mar 21, 2026
3ea07ba
test: set cashier on payment in BillServiceImplTest
NethmiRodrigo Mar 21, 2026
e821245
feat: auto-set cashier on payment from authenticated user; expose in …
NethmiRodrigo Mar 21, 2026
2f04574
fix: run cashier validation before line-item DB query to avoid Hibern…
NethmiRodrigo Mar 21, 2026
44630e6
Allow client to set cashier
NethmiRodrigo Mar 24, 2026
ec2141e
Update liquibase
NethmiRodrigo Mar 24, 2026
e764716
test: move validator tests to BillValidatorTest and add voided paymen…
NethmiRodrigo Mar 25, 2026
bfd4f72
test: verify BillValidator cashier check is triggered via saveBill; r…
NethmiRodrigo Mar 25, 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
10 changes: 8 additions & 2 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 @@ -13,14 +13,16 @@
*/
package org.openmrs.module.billing.api.model;

import java.math.BigDecimal;

import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.Setter;
import org.openmrs.module.billing.api.base.entity.model.BaseInstanceCustomizableData;

import java.math.BigDecimal;
import org.openmrs.Provider;
import org.openmrs.module.billing.api.base.entity.model.BaseInstanceCustomizableData;

/**
* Model class that represents the {@link Bill} payment information.
Expand All @@ -46,6 +48,10 @@ public class Payment extends BaseInstanceCustomizableData<PaymentMode, PaymentAt
@Setter
private BigDecimal amountTendered;

@Getter
@Setter
private Provider cashier;

public Integer getId() {
return paymentId;
}
Expand Down
18 changes: 18 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 @@ -35,6 +36,7 @@ public void validate(@Nonnull Object target, @Nonnull Errors errors) {
errors.rejectValue("voided", "error.null");
}

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

/**
* Validates that any new (unsaved) non-voided payment has a cashier. Existing persisted payments
* (id != null) are exempt to allow legacy data.
*/
private void validateNewPaymentsHaveCashier(Bill bill, Errors errors) {
if (bill.getPayments() == null) {
return;
}
for (Payment payment : bill.getPayments()) {
if (payment != null && !payment.getVoided() && payment.getId() == null && payment.getCashier() == null) {
errors.reject("billing.error.paymentCashierRequired");
return;
}
}
}

}
1 change: 1 addition & 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 @@ -199,6 +199,7 @@

<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"/>
Expand Down
1 change: 1 addition & 0 deletions api/src/main/resources/messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ openhmis.cashier.payment.error.paymentMode.required=Payment mode is required.
openhmis.cashier.payment.error.amountType=Amount needs to be a number
openhmis.cashier.payment.error.amountRequired=Amount is required.
openhmis.cashier.payment.confirm.paymentProcess=Are you sure you want to process a %s payment of %s?
billing.error.paymentCashierRequired=Each payment must have an associated cashier.
#setting page
openhmis.cashier.setting.header=Cashier Settings
openhmis.cashier.setting.adjustmentReason.field.header=Require Adjustment Reason
Expand Down
35 changes: 35 additions & 0 deletions 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 @@ -21,6 +21,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.openmrs.Patient;
import org.openmrs.Provider;
import org.openmrs.api.PatientService;
import org.openmrs.api.ProviderService;
import org.openmrs.api.UnchangeableObjectException;
Expand Down Expand Up @@ -323,6 +324,37 @@ public void saveBill_shouldNotAllowChangesForPostedBill() {
assertThrows(UnchangeableObjectException.class, Context::flushSession);
}

@Test
public void saveBill_shouldThrowExceptionWhenNewPaymentHasNoCashier() {
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);
// cashier intentionally NOT set

postedBill.addPayment(payment);

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

@Test
public void saveBill_shouldTolerateExistingPaymentsWithNoCashier() {
// Legacy payments (with ID) that have no cashier must be tolerated
Bill paidBill = billService.getBill(1);
assertNotNull(paidBill);
assertFalse(paidBill.getPayments().isEmpty());

Payment existingPayment = paidBill.getPayments().iterator().next();
assertNotNull(existingPayment.getId(), "Existing payment should have an ID");

assertDoesNotThrow(() -> billService.saveBill(paidBill));
}

@Test
public void saveBill_shouldAllowPaymentsForPostedBill() {
Bill postedBill = billService.getBill(0);
Expand All @@ -331,9 +363,12 @@ public void saveBill_shouldAllowPaymentsForPostedBill() {

PaymentMode paymentMode = paymentModeService.getPaymentMode(0);

Provider cashier = Context.getProviderService().getProvider(1);

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

postedBill.addPayment(payment);
billService.saveBill(postedBill);
Expand Down
1 change: 1 addition & 0 deletions api/src/test/java/org/openmrs/module/billing/validator/BillValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,5 @@ public void validate_shouldNotRejectUnmodifiedPaidBill() {
// attempting to modify line items
assertFalse(errors.hasErrors());
}

Copy link
Member

@wikumChamith wikumChamith Mar 24, 2026

Choose a reason for hiding this comment

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

It would be great to have some actual tests here :)

Copy link
Contributor Author

@NethmiRodrigo NethmiRodrigo Mar 24, 2026

Choose a reason for hiding this comment

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

Don’t the tests in BillServiceImplTest.java‎ make sure that the validator works too?

Copy link
Member

@wikumChamith wikumChamith Mar 25, 2026

Choose a reason for hiding this comment

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

Yeah but having some seperate tests for the validator is convenient.

Copy link
Contributor Author

@NethmiRodrigo NethmiRodrigo Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Convenient, how? That would be redundant. Since the validator is already getting tested, having this just means adding another test that tests the same thing. It already takes a while to run tests, why add more if not needed

Copy link
Member

@wikumChamith wikumChamith Mar 25, 2026

Choose a reason for hiding this comment

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

By that logic, we shouldn't write tests for the DAO layer since the service layer already covers it. The point of unit tests is to test a unit in isolation, so failures are obvious and edge cases are cheap to cover.

Copy link
Contributor Author

@NethmiRodrigo NethmiRodrigo Mar 25, 2026

Choose a reason for hiding this comment

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

@wikumChamith I moved the tests to the validator :)

}
8 changes: 8 additions & 0 deletions omod/src/main/java/org/openmrs/module/billing/web/rest/resource/BillResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ public void setBillPayments(Bill instance, Set<Payment> payments) {
}
BaseRestDataResource.syncCollection(instance.getPayments(), payments);
for (Payment payment : instance.getPayments()) {
if (payment.getId() == null && payment.getCashier() == null) {
Provider cashier = getCurrentCashier();
Copy link
Member

@wikumChamith wikumChamith Mar 24, 2026

Choose a reason for hiding this comment

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

Can't we use ProviderUtil.getCurrentProvider() here?

Copy link
Contributor Author

@NethmiRodrigo NethmiRodrigo Mar 24, 2026

Choose a reason for hiding this comment

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

getCurrentCashier() method is the same logic:

  • both use Context.getAuthenticatedUser().getPerson()
  • both call ProviderService.getProvidersByPerson(...)
  • both return first provider or null

Copy link
Member

@wikumChamith wikumChamith Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, why are we duplicating existing functionalities? :)

Copy link
Member

@wikumChamith wikumChamith Mar 27, 2026

Choose a reason for hiding this comment

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

@NethmiRodrigo Since they're the same logic, can you replace getCurrentCashier() with ProviderUtil.getCurrentProvider() here to avoid the duplication?

if (cashier == null) {
throw new RestClientException("Couldn't find Provider for the current user ("
+ Context.getAuthenticatedUser().getUsername() + ")");
}
payment.setCashier(cashier);
}
instance.addPayment(payment);
}
}
Expand Down
23 changes: 23 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,8 +13,11 @@
*/
package org.openmrs.module.billing.web.rest.resource;

import org.openmrs.Provider;
import org.openmrs.api.APIException;
import org.openmrs.api.context.Context;
import org.openmrs.module.billing.api.BillService;
import org.openmrs.module.billing.api.base.ProviderUtil;
import org.openmrs.module.billing.api.PaymentModeService;
import org.openmrs.module.billing.web.base.resource.BaseRestDataResource;
import org.openmrs.module.billing.api.model.Bill;
Expand Down Expand Up @@ -55,6 +58,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 +74,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 Expand Up @@ -132,6 +146,15 @@ public Long getPaymentDate(Payment instance) {

@Override
public Payment save(Payment delegate) {
if (delegate.getCashier() == null) {
Provider cashier = ProviderUtil.getCurrentProvider();
if (cashier == null) {
throw new APIException(
"The authenticated user is not associated with a Provider and cannot process payments.");
}
delegate.setCashier(cashier);
}

BillService service = Context.getService(BillService.class);
Bill bill = delegate.getBill();
bill.addPayment(delegate);
Expand Down
12 changes: 12 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,16 @@
baseTableName="bill_exemption_rule" baseColumnNames="voided_by"
referencedTableName="users" referencedColumnNames="user_id"/>
</changeSet>

<changeSet id="openmrs.billing-004-20260320-add-provider-id-to-bill-payment" author="Nethmi Rodrigo">
<comment>Add cashier (provider) for each payment to track which cashier processed the payment</comment>
<addColumn tableName="cashier_bill_payment">
<column name="provider_id" type="int"/>
<constraints nullable="true"/>
Copy link
Member

@wikumChamith wikumChamith Mar 24, 2026

Choose a reason for hiding this comment

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

Constraint should be inside the column tag

  <column name="provider_id" type="int">
      <constraints nullable="true"/>
  </column>

Copy link
Contributor Author

@NethmiRodrigo NethmiRodrigo Mar 24, 2026

Choose a reason for hiding this comment

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

@claude[agent] do this change in this PR

Copy link
Contributor Author

@NethmiRodrigo NethmiRodrigo Mar 24, 2026

Choose a reason for hiding this comment

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

Nvm my session credits are over sigh

</addColumn>
<addForeignKeyConstraint
constraintName="cashier_bill_payment_provider_id_fk"
baseTableName="cashier_bill_payment" baseColumnNames="provider_id"
referencedTableName="provider" referencedColumnNames="provider_id"/>
</changeSet>
</databaseChangeLog>
Copy link
Member

@wikumChamith wikumChamith Mar 27, 2026

Choose a reason for hiding this comment

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

Missing newline.

Loading