Skip to content

Comments

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

Open
wikumChamith wants to merge 2 commits intoopenmrs:mainfrom
wikumChamith:O3-5197
Open

O3-5197: Each payment should have an associated Cashier#109
wikumChamith wants to merge 2 commits intoopenmrs:mainfrom
wikumChamith:O3-5197

Conversation

@wikumChamith
Copy link
Member

  • Adds a cashier (Provider) field to Payment so each payment tracks which cashier processed it
  • Validates that all new payments have an associated cashier via BillValidator AOP
  • Exposes cashier in the Payment REST resource for create and read operations
  • Adds Liquibase migration to add provider_id column to cashier_bill_payment table (nullable for legacy data)


@Getter
@Setter
private Provider cashier;
Copy link
Member

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

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.

Copy link
Contributor

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

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

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

}
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

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

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

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

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.

@wikumChamith wikumChamith requested a review from ibacher February 10, 2026 15:56
@wikumChamith
Copy link
Member Author

@ibacher any updates on this?

@wikumChamith wikumChamith requested a review from ibacher February 11, 2026 16:32
@NethmiRodrigo
Copy link
Contributor

Can we hold off on this until we get the release out of the way in the case that this might require changes to the frontend? Unless the cashier is automatically set from the context authenticated user? Also, do we do that, and if not, why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants