Skip to content

Conversation

@abertnamanya
Copy link
Member

@abertnamanya abertnamanya commented Oct 16, 2025

Requirements

  • This PR has a title that briefly describes the work done including a conventional commit type prefix and a Jira ticket number if applicable. See existing PR titles for inspiration.
  • My work is based on designs, which are linked or shown either in the Jira ticket or the description below.
  • My work includes tests or is validated by existing tests.

Summary

Edit/update cashpoints in the billing module

Screenshots

Screen-Recording.5.mp4

Related Issue

https://openmrs.atlassian.net/browse/O3-5121

Other

Copy link
Contributor

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

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

Thanks @abertnamanya, a few suggestions:

@NethmiRodrigo NethmiRodrigo changed the title O3-5121:Edit/Update Cashpoint (feat) O3-5121: Allow users to edit a cashpoint Oct 16, 2025
@denniskigen
Copy link
Member

denniskigen commented Oct 16, 2025

Does the backend resource fully support editing cash points? @dkayiwa

@dkayiwa
Copy link
Member

dkayiwa commented Oct 16, 2025

@denniskigen no it does not.

@denniskigen
Copy link
Member

So we'd need to fix that as a pre-requisite for any work on the frontend, right?

@denniskigen
Copy link
Member

denniskigen commented Oct 21, 2025

Actually, it looks like the save functionality exists in the base class here https://github.com/openmrs/openmrs-module-billing/blob/main/omod/src/main/java/org/openmrs/module/billing/web/base/resource/BaseRestMetadataResource.java#L61, so this should be viable. I'd recommend dropping the UUID field entirely from the form and also verifying that the ModalHeader changes based on the context in which the form is used. From the video, it says "Add cash point" for both the create and edit flows.

@NethmiRodrigo
Copy link
Contributor

NethmiRodrigo commented Oct 21, 2025

I'd recommend dropping the UUID field entirely from the form

@denniskigen I think its okay to leave this as is because cash points are mainly meant to be an Iniz config like so - https://github.com/openmrs/openmrs-content-referenceapplication-demo/blob/main/configuration/backend_configuration/cashpoints/cashPoints.csv

The current implementation allows editing cash points through the UI, but these changes:

  • Only update the database - not the original configuration files
  • Create configuration drift between CSV files and database state
  • Will be lost during system rebuilds or deployments
  • Don't propagate to other environments

Maybe we could add a clear warning indicating that changes made to the cash points configuration are ephemeral and that the canonical place to make changes is in the demo content package.

Any thoughts, @ibacher?

@denniskigen denniskigen changed the title (feat) O3-5121: Allow users to edit a cashpoint (feat) O3-5121: Add ability to edit a cashpoint Oct 22, 2025
@denniskigen denniskigen changed the title (feat) O3-5121: Add ability to edit a cashpoint (feat) O3-5121: Add ability to edit cashpoints Oct 22, 2025

export const updateCashPoint = (uuid: string, payload: UpdateCashPointPayload) => {
return openmrsFetch(`${apiBasePath}cashPoint/${uuid}`, {
method: 'POST',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
method: 'POST',
method: 'PUT',

PUT is the canonical HTTP request method for updates.

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, it should be a PUT, but the endpoint does not seem to accept the request

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that sounds like a bug. The controller in the backend might be misparsing main resource URLs. Could you have a look when you get some time, @wikumChamith?

Let's keep it as a POST for now, @abertnamanya.

@denniskigen
Copy link
Member

So, the code LGTM. What's holding this up is the potential config drift issue I spoke about here. Still eager to hear the thoughts of @dkayiwa @wikumChamith @ibacher on how we should handle it.

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.

4 participants