O3-5413: Add bill refund request and approval workflow

[UjjawalPrabhat]

Summary

This PR adds backend support for the frontend to enable refund requests and approval for paid bills.

• Add REFUND_REQUESTED and REFUNDED to BillStatus enum
• Allow PAID → REFUND_REQUESTED and REFUND_REQUESTED → REFUNDED status transitions, enforcing REFUND_MONEY privilege for issuing refunds
• Guard synchronizeBillStatus() from overwriting refund statuses during payment recalculation

Related Issue

O3-5413

[UjjawalPrabhat]

@NethmiRodrigo @wikumChamith @ibacher Could you please review this PR?

[ELVIS-KATO]

@UjjawalPrabhat thank you for the work done, the changes LGTM, but if you don't mind, For when rejecting a refund request (REFUND_REQUESTED → PAID), consider clearing the refund request metadata for clarity. (Optional but could make the system cleaner when cleared)

[NethmiRodrigo]

Thanks @UjjawalPrabhat, but can we have a separate endpoint to handle refunds ?

[NethmiRodrigo]

consider clearing the refund request metadata for clarity. (Optional but could make the system cleaner when cleared)

The whole point of recording that metadata is to have this record for reporting or auditing purposes so clearing it would be bad

[codecov-commenter]

Codecov Report

❌ Patch coverage is 65.21739% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.65%. Comparing base (b52412a) to head (0959daf).

Files with missing lines	Patch %	Lines
...module/billing/web/rest/resource/BillResource.java	0.00%	24 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #108      +/-   ##
============================================
+ Coverage     27.04%   27.65%   +0.61%     
- Complexity      497      515      +18     
============================================
  Files           190      190              
  Lines          4337     4400      +63     
  Branches        492      505      +13     
============================================
+ Hits           1173     1217      +44     
- Misses         3062     3081      +19     
  Partials        102      102              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[NethmiRodrigo]

@UjjawalPrabhat please test your changes out locally, since this fails on runtime.
@VeronicaMuthee if a refund request gets denied, what should the workflow be? Should we set the bill status back to paid or to a new status saying refund denied? And can the refund request reason be null, i.e should we allow a cashier to request for a refund without specifying a reason?

[NethmiRodrigo]

@VeronicaMuthee if a refund request gets denied, what should the workflow be? Should we set the bill status back to paid or to a new status saying refund denied? And can the refund request reason be null, i.e should we allow a cashier to request for a refund without specifying a reason?

Reply from @VeronicaMuthee in the ticket:

Refund denied because it makes it clear to the cashier that a refund was requested and explicitly rejected.
Yes, the denial reason should be saved for accountability and audit trails.
I'd recommend making the reason required

[ibacher]

We don't use conventional commits on backend modules, so please remove that from the PR title.

[NethmiRodrigo]

Thanks @UjjawalPrabhat! Continuing to enjoy the work you're bringing to this module — the service layer design is solid and the test coverage there is thorough. A few things to sort out before we merge, mainly around wiring the REST layer up to the service methods you wrote.

[NethmiRodrigo]

@UjjawalPrabhat — heads up, I wasn't able to run your changes locally because the liquibase changeset used modifyDataType which wasn’t available in the used version of liquibase.

I previously mentioned in this same PR to please test your changes locally. You can use the sdk, just setup a sdk and then deploy this module to the sdk, then run the sdk and you should have an openmrs instance running with your changes. It’s just -

1. mvn openmrs-sdk:setup
2. mvn openmrs-sdk:deploy in the project directory after you’ve done a build
3. mvn openmrs-sdk:run

Had I not run this locally and checked we wouldn’t have known it failed at runtime, which is bad

[NethmiRodrigo]

@ibacher @wikumChamith could you review as well? I updated the PR because

• Refund flow has a setter/guard conflict. BillResource.setBillStatus mutates the in-memory status to the target during JSON binding (e.g. PAID → REFUND_REQUESTED), but the service-layer guards (requestRefund/approveRefund/rejectRefund) asserted on the source status. So the PUT flow was throwing IllegalArgumentException every time.

I’ve added BillDAO.getPersistedBillStatus(Integer) / BillService.getPersistedBillStatus to HibernateBillDao, similar to the existing BillLineItemDAO.getLineItemIdsByBillId pattern used by validateLineItemsNotModified.

[NethmiRodrigo]

Also, FYI @ibacher, the build job is failing to start

Invalid workflow file: .github/workflows/build.yml#L15
The workflow is not valid. .github/workflows/build.yml (Line: 15, Col: 3): Error calling workflow 'openmrs/openmrs-contrib-gha-workflows/.github/workflows/build-backend-module.yml@main'. The nested job 'build' is requesting 'id-token: write', but is only allowed 'id-token: none'.

[wikumChamith]

This will change the whole pattern. What if we introduce a new data model for refunds? That's the same approach we're taking for payments.

PROS:

• It's more natural with the current OpenMRS codebase
• Multiple/partial refunds become representable
• We can keep a full refund history instead of just the latest state

CONS:

We need to start again :)

@ibacher, @NethmiRodrigo , what do you think?

[NethmiRodrigo]

This will change the whole pattern. What if we introduce a new data model for refunds? That's the same approach we're taking for payments.

PROS:

• It's more natural with the current OpenMRS codebase

• Multiple/partial refunds become representable

• We can keep a full refund history instead of just the latest state

CONS:

We need to start again :)

@ibacher, @NethmiRodrigo , what do you think?

So, right now we have a requirement scenario for refunding only a fully paid bill. I was okay with this approach because we don't yet have a solid requirement for partial refunds, and because partial refunds can get complicated. The same reason I'm not exactly a huge fan of our payment model right now. If there is a bill with multiple items, refunding a specific amount might mean refunding for a specific line item, (which is something we can support). But we don't track which payment was made for which line item.
My opinion is let's go with this for now until we get community feedback and requirements. That said, I'm more than okay with the con, I'd rather us making something right :)

[wikumChamith]

This will change the whole pattern. What if we introduce a new data model for refunds? That's the same approach we're taking for payments.
PROS:

• It's more natural with the current OpenMRS codebase
• Multiple/partial refunds become representable
• We can keep a full refund history instead of just the latest state

CONS:
We need to start again :)
@ibacher, @NethmiRodrigo , what do you think?

So, right now we have a requirement scenario for refunding only a fully paid bill. I was okay with this approach because we don't yet have a solid requirement for partial refunds, and because partial refunds can get complicated. The same reason I'm not exactly a huge fan of our payment model right now. If there is a bill with multiple items, refunding a specific amount might mean refunding for a specific line item, (which is something we can support). But we don't track which payment was made for which line item. My opinion is let's go with this for now until we get community feedback and requirements. That said, I'm more than okay with the con, I'd rather us making something right :)

I actually think paying for individual items separately doesn't make sense. I've never seen something like that. In most systems, you just create a separate bill to do that. I think we should get rid of the one bill per patient rule.

[ibacher]

Refund flow has a setter/guard conflict.

Because of the semi-strict status state machine in this module maybe we shouldn't allow the frontend to control this at all, but just provide operations, e.g., /bill/<uuid>/refund that will eventually set these if preconditions are met?

But we don't track which payment was made for which line item.

I don't think it matters which payment is made towards which line-item, but we can separately track the line items associated with a refund. Paying for things individually on a single bill doesn't make much sense, but refunding individual items probably needs to be supported (with the ability to mark a refund as applying to the whole bill).

[NethmiRodrigo]

I think we should get rid of the one bill per patient rule.

@wikumChamith Actually, we accidentally did, lol, happened after that large refactor somewhere.

refunding individual items probably needs to be supported (with the ability to mark a refund as applying to the whole bill).

@ibacher in that case, we would need to update the status of the individual line items right, and allow cashiers to select the line item that they want to request a refund for? Do we only change the status of the entire bill, if all line items are selected for refunds?

[NethmiRodrigo]

@VeronicaMuthee pinging you just to keep you in the loop on the ^ conversation :)

[wikumChamith]

I think we should get rid of the one bill per patient rule.

@wikumChamith Actually, we accidentally did, lol, happened after that large refactor somewhere.

refunding individual items probably needs to be supported (with the ability to mark a refund as applying to the whole bill).

@ibacher in that case, we would need to update the status of the individual line items right, and allow cashiers to select the line item that they want to request a refund for? Do we only change the status of the entire bill, if all line items are selected for refunds?

The best option here is to implement this in a more flexible way, so we have more wiggle room in the future. That's why I suggested having a separate data model for refunds.
Also, something interesting I noticed while working with Odoo is that refunds are treated as negative payments. That might be something we can take into account as well.

[NethmiRodrigo]

The best option here is to implement this in a more flexible way, so we have more wiggle room in the future. That's why I suggested having a separate data model for refunds.

Alrighty, lets do it this way then